Skip to content

Commit 55dfd45

Browse files
ndosschendossche
authored
Fix bug #63937: Upload speed 10 times slower with PHP (#13041)
There are two slow parts in the upload logic: - Reading from the input stream character by character - Checking each character one by one to normalize line endings First of all, the line normalization isn't necessary for binary transfers, so we can use a simple read while loop to read bytes into the transfer buffer. Second, for the ASCII transfer where we do have to normalize line endings, we can be smarter than reading one character at a time. There's a php_stream_get_line() function that we can repurpose if the flags for the stream are set up properly. This patch implements these fixes. Results: I tested this on an 850 MiB file, transferring this to an FTP server running locally. Results before patch: Binary/ASCII transfer (same code path): 8.21s Results after patch: Binary transfer: 0.65s ASCII transfer: 0.74s Further improvement is probably possible by having a larger send buffer.
1 parent 08b2ab2 commit 55dfd45

File tree

2 files changed

+115
-75
lines changed

2 files changed

+115
-75
lines changed

‎ext/ftp/ftp.c‎

Lines changed: 82 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -974,14 +974,84 @@ ftp_get(ftpbuf_t *ftp, php_stream *outstream, const char *path, const size_t pat
974974
}
975975
/* }}} */
976976

977+
static zend_result ftp_send_stream_to_data_socket(ftpbuf_t *ftp, databuf_t *data, php_stream *instream, ftptype_t type, bool send_once_and_return)
978+
{
979+
if (type == FTPTYPE_ASCII) {
980+
/* Change (and later restore) flags to make sure php_stream_get_line() searches '\n'. */
981+
const uint32_t flags_mask = PHP_STREAM_FLAG_EOL_UNIX | PHP_STREAM_FLAG_DETECT_EOL | PHP_STREAM_FLAG_EOL_MAC;
982+
uint32_t old_flags = instream->flags & flags_mask;
983+
instream->flags = (instream->flags & ~flags_mask) | PHP_STREAM_FLAG_EOL_UNIX;
984+
985+
char *ptr = data->buf;
986+
const char *end = data->buf + FTP_BUFSIZE;
987+
while (!php_stream_eof(instream)) {
988+
size_t line_length;
989+
if (!php_stream_get_line(instream, ptr, end - ptr, &line_length)) {
990+
break;
991+
}
992+
993+
ZEND_ASSERT(line_length != 0);
994+
995+
ptr += line_length - 1;
996+
/* Replace \n with \r\n */
997+
if (*ptr == '\n') {
998+
*ptr = '\r';
999+
/* The streams layer always puts a \0 byte at the end of a line,
1000+
* so there is always place to add an extra byte. */
1001+
*++ptr = '\n';
1002+
}
1003+
1004+
ptr++;
1005+
1006+
/* If less than 2 bytes remain, either the buffer is completely full or there is a single byte left to put a '\0'
1007+
* which isn't really useful, in this case send and reset the buffer. */
1008+
if (end - ptr < 2) {
1009+
size_t send_size = FTP_BUFSIZE - (end - ptr);
1010+
if (UNEXPECTED(my_send(ftp, data->fd, data->buf, send_size) != send_size)) {
1011+
instream->flags = (instream->flags & ~flags_mask) | old_flags;
1012+
return FAILURE;
1013+
}
1014+
ptr = data->buf;
1015+
if (send_once_and_return) {
1016+
break;
1017+
}
1018+
}
1019+
}
1020+
1021+
instream->flags = (instream->flags & ~flags_mask) | old_flags;
1022+
1023+
if (end - ptr < FTP_BUFSIZE) {
1024+
size_t send_size = FTP_BUFSIZE - (end - ptr);
1025+
if (UNEXPECTED(my_send(ftp, data->fd, data->buf, send_size) != send_size)) {
1026+
return FAILURE;
1027+
}
1028+
}
1029+
} else {
1030+
while (!php_stream_eof(instream)) {
1031+
ssize_t size = php_stream_read(instream, data->buf, FTP_BUFSIZE);
1032+
if (size == 0) {
1033+
break;
1034+
}
1035+
if (UNEXPECTED(size < 0)) {
1036+
return FAILURE;
1037+
}
1038+
if (UNEXPECTED(my_send(ftp, data->fd, data->buf, size) != size)) {
1039+
return FAILURE;
1040+
}
1041+
if (send_once_and_return) {
1042+
break;
1043+
}
1044+
}
1045+
}
1046+
1047+
return SUCCESS;
1048+
}
1049+
9771050
/* {{{ ftp_put */
9781051
int
9791052
ftp_put(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *instream, ftptype_t type, zend_long startpos)
9801053
{
9811054
databuf_t *data = NULL;
982-
zend_long size;
983-
char *ptr;
984-
int ch;
9851055
char arg[MAX_LENGTH_OF_LONG];
9861056

9871057
if (ftp == NULL) {
@@ -1018,30 +1088,10 @@ ftp_put(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *inst
10181088
goto bail;
10191089
}
10201090

1021-
size = 0;
1022-
ptr = data->buf;
1023-
while (!php_stream_eof(instream) && (ch = php_stream_getc(instream))!=EOF) {
1024-
/* flush if necessary */
1025-
if (FTP_BUFSIZE - size < 2) {
1026-
if (my_send(ftp, data->fd, data->buf, size) != size) {
1027-
goto bail;
1028-
}
1029-
ptr = data->buf;
1030-
size = 0;
1031-
}
1032-
1033-
if (ch == '\n' && type == FTPTYPE_ASCII) {
1034-
*ptr++ = '\r';
1035-
size++;
1036-
}
1037-
1038-
*ptr++ = ch;
1039-
size++;
1040-
}
1041-
1042-
if (size && my_send(ftp, data->fd, data->buf, size) != size) {
1091+
if (ftp_send_stream_to_data_socket(ftp, data, instream, type, false) != SUCCESS) {
10431092
goto bail;
10441093
}
1094+
10451095
data_close(ftp);
10461096

10471097
if (!ftp_getresp(ftp) || (ftp->resp != 226 && ftp->resp != 250 && ftp->resp != 200)) {
@@ -1060,9 +1110,6 @@ int
10601110
ftp_append(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *instream, ftptype_t type)
10611111
{
10621112
databuf_t *data = NULL;
1063-
zend_long size;
1064-
char *ptr;
1065-
int ch;
10661113

10671114
if (ftp == NULL) {
10681115
return 0;
@@ -1085,30 +1132,10 @@ ftp_append(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *i
10851132
goto bail;
10861133
}
10871134

1088-
size = 0;
1089-
ptr = data->buf;
1090-
while (!php_stream_eof(instream) && (ch = php_stream_getc(instream))!=EOF) {
1091-
/* flush if necessary */
1092-
if (FTP_BUFSIZE - size < 2) {
1093-
if (my_send(ftp, data->fd, data->buf, size) != size) {
1094-
goto bail;
1095-
}
1096-
ptr = data->buf;
1097-
size = 0;
1098-
}
1099-
1100-
if (ch == '\n' && type == FTPTYPE_ASCII) {
1101-
*ptr++ = '\r';
1102-
size++;
1103-
}
1104-
1105-
*ptr++ = ch;
1106-
size++;
1107-
}
1108-
1109-
if (size && my_send(ftp, data->fd, data->buf, size) != size) {
1135+
if (ftp_send_stream_to_data_socket(ftp, data, instream, type, false) != SUCCESS) {
11101136
goto bail;
11111137
}
1138+
11121139
data_close(ftp);
11131140

11141141
if (!ftp_getresp(ftp) || (ftp->resp != 226 && ftp->resp != 250 && ftp->resp != 200)) {
@@ -2256,39 +2283,19 @@ ftp_nb_put(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *i
22562283
int
22572284
ftp_nb_continue_write(ftpbuf_t *ftp)
22582285
{
2259-
long size;
2260-
char *ptr;
2261-
int ch;
2262-
22632286
/* check if we can write more data */
22642287
if (!data_writeable(ftp, ftp->data->fd)) {
22652288
return PHP_FTP_MOREDATA;
22662289
}
22672290

2268-
size = 0;
2269-
ptr = ftp->data->buf;
2270-
while (!php_stream_eof(ftp->stream) && (ch = php_stream_getc(ftp->stream)) != EOF) {
2271-
2272-
if (ch == '\n' && ftp->type == FTPTYPE_ASCII) {
2273-
*ptr++ = '\r';
2274-
size++;
2275-
}
2276-
2277-
*ptr++ = ch;
2278-
size++;
2279-
2280-
/* flush if necessary */
2281-
if (FTP_BUFSIZE - size < 2) {
2282-
if (my_send(ftp, ftp->data->fd, ftp->data->buf, size) != size) {
2283-
goto bail;
2284-
}
2285-
return PHP_FTP_MOREDATA;
2286-
}
2291+
if (ftp_send_stream_to_data_socket(ftp, ftp->data, ftp->stream, ftp->type, true) != SUCCESS) {
2292+
goto bail;
22872293
}
22882294

2289-
if (size && my_send(ftp, ftp->data->fd, ftp->data->buf, size) != size) {
2290-
goto bail;
2295+
if (!php_stream_eof(ftp->stream)) {
2296+
return PHP_FTP_MOREDATA;
22912297
}
2298+
22922299
data_close(ftp);
22932300

22942301
if (!ftp_getresp(ftp) || (ftp->resp != 226 && ftp->resp != 250)) {

‎ext/ftp/tests/ftp_fput_ascii_over_4_kib.phpt‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
--TEST--
2+
Testing ftp_fput basic functionality
3+
--EXTENSIONS--
4+
ftp
5+
pcntl
6+
--FILE--
7+
<?php
8+
require 'server.inc';
9+
10+
$ftp = ftp_connect('127.0.0.1', $port);
11+
ftp_login($ftp, 'user', 'pass');
12+
$ftp or die("Couldn't connect to the server");
13+
14+
$filename = "large_file.txt";
15+
$filepath = __DIR__ . "/" . $filename;
16+
17+
// Test on boundary of 2 buffers
18+
for ($i = 4094; $i < 4098; $i++) {
19+
$contents = str_repeat("a", $i) . "\n" . str_repeat("b", 10);
20+
file_put_contents($filepath, $contents);
21+
var_dump(ftp_put($ftp, "large_file.txt", $filepath, FTP_ASCII));
22+
}
23+
24+
?>
25+
--CLEAN--
26+
<?php
27+
@unlink(__DIR__ . "/large_file.txt");
28+
?>
29+
--EXPECT--
30+
bool(true)
31+
bool(true)
32+
bool(true)
33+
bool(true)

0 commit comments

Comments
 (0)