From 55dfd455814c8490220143e50b90011fe5ff1d72 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 13 Apr 2024 01:39:31 +0200 Subject: [PATCH] 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. --- ext/ftp/ftp.c | 159 ++++++++++--------- ext/ftp/tests/ftp_fput_ascii_over_4_kib.phpt | 33 ++++ 2 files changed, 116 insertions(+), 76 deletions(-) create mode 100644 ext/ftp/tests/ftp_fput_ascii_over_4_kib.phpt diff --git a/ext/ftp/ftp.c b/ext/ftp/ftp.c index 1d82cf43e06..908130d62aa 100644 --- a/ext/ftp/ftp.c +++ b/ext/ftp/ftp.c @@ -974,14 +974,84 @@ bail: } /* }}} */ +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) +{ + if (type == FTPTYPE_ASCII) { + /* Change (and later restore) flags to make sure php_stream_get_line() searches '\n'. */ + const uint32_t flags_mask = PHP_STREAM_FLAG_EOL_UNIX | PHP_STREAM_FLAG_DETECT_EOL | PHP_STREAM_FLAG_EOL_MAC; + uint32_t old_flags = instream->flags & flags_mask; + instream->flags = (instream->flags & ~flags_mask) | PHP_STREAM_FLAG_EOL_UNIX; + + char *ptr = data->buf; + const char *end = data->buf + FTP_BUFSIZE; + while (!php_stream_eof(instream)) { + size_t line_length; + if (!php_stream_get_line(instream, ptr, end - ptr, &line_length)) { + break; + } + + ZEND_ASSERT(line_length != 0); + + ptr += line_length - 1; + /* Replace \n with \r\n */ + if (*ptr == '\n') { + *ptr = '\r'; + /* The streams layer always puts a \0 byte at the end of a line, + * so there is always place to add an extra byte. */ + *++ptr = '\n'; + } + + ptr++; + + /* If less than 2 bytes remain, either the buffer is completely full or there is a single byte left to put a '\0' + * which isn't really useful, in this case send and reset the buffer. */ + if (end - ptr < 2) { + size_t send_size = FTP_BUFSIZE - (end - ptr); + if (UNEXPECTED(my_send(ftp, data->fd, data->buf, send_size) != send_size)) { + instream->flags = (instream->flags & ~flags_mask) | old_flags; + return FAILURE; + } + ptr = data->buf; + if (send_once_and_return) { + break; + } + } + } + + instream->flags = (instream->flags & ~flags_mask) | old_flags; + + if (end - ptr < FTP_BUFSIZE) { + size_t send_size = FTP_BUFSIZE - (end - ptr); + if (UNEXPECTED(my_send(ftp, data->fd, data->buf, send_size) != send_size)) { + return FAILURE; + } + } + } else { + while (!php_stream_eof(instream)) { + ssize_t size = php_stream_read(instream, data->buf, FTP_BUFSIZE); + if (size == 0) { + break; + } + if (UNEXPECTED(size < 0)) { + return FAILURE; + } + if (UNEXPECTED(my_send(ftp, data->fd, data->buf, size) != size)) { + return FAILURE; + } + if (send_once_and_return) { + break; + } + } + } + + return SUCCESS; +} + /* {{{ ftp_put */ int ftp_put(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *instream, ftptype_t type, zend_long startpos) { databuf_t *data = NULL; - zend_long size; - char *ptr; - int ch; char arg[MAX_LENGTH_OF_LONG]; if (ftp == NULL) { @@ -1018,30 +1088,10 @@ ftp_put(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *inst goto bail; } - size = 0; - ptr = data->buf; - while (!php_stream_eof(instream) && (ch = php_stream_getc(instream))!=EOF) { - /* flush if necessary */ - if (FTP_BUFSIZE - size < 2) { - if (my_send(ftp, data->fd, data->buf, size) != size) { - goto bail; - } - ptr = data->buf; - size = 0; - } - - if (ch == '\n' && type == FTPTYPE_ASCII) { - *ptr++ = '\r'; - size++; - } - - *ptr++ = ch; - size++; - } - - if (size && my_send(ftp, data->fd, data->buf, size) != size) { + if (ftp_send_stream_to_data_socket(ftp, data, instream, type, false) != SUCCESS) { goto bail; } + data_close(ftp); if (!ftp_getresp(ftp) || (ftp->resp != 226 && ftp->resp != 250 && ftp->resp != 200)) { @@ -1060,9 +1110,6 @@ int ftp_append(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *instream, ftptype_t type) { databuf_t *data = NULL; - zend_long size; - char *ptr; - int ch; if (ftp == NULL) { return 0; @@ -1085,30 +1132,10 @@ ftp_append(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *i goto bail; } - size = 0; - ptr = data->buf; - while (!php_stream_eof(instream) && (ch = php_stream_getc(instream))!=EOF) { - /* flush if necessary */ - if (FTP_BUFSIZE - size < 2) { - if (my_send(ftp, data->fd, data->buf, size) != size) { - goto bail; - } - ptr = data->buf; - size = 0; - } - - if (ch == '\n' && type == FTPTYPE_ASCII) { - *ptr++ = '\r'; - size++; - } - - *ptr++ = ch; - size++; - } - - if (size && my_send(ftp, data->fd, data->buf, size) != size) { + if (ftp_send_stream_to_data_socket(ftp, data, instream, type, false) != SUCCESS) { goto bail; } + data_close(ftp); if (!ftp_getresp(ftp) || (ftp->resp != 226 && ftp->resp != 250 && ftp->resp != 200)) { @@ -2256,39 +2283,19 @@ bail: int ftp_nb_continue_write(ftpbuf_t *ftp) { - long size; - char *ptr; - int ch; - /* check if we can write more data */ if (!data_writeable(ftp, ftp->data->fd)) { return PHP_FTP_MOREDATA; } - size = 0; - ptr = ftp->data->buf; - while (!php_stream_eof(ftp->stream) && (ch = php_stream_getc(ftp->stream)) != EOF) { - - if (ch == '\n' && ftp->type == FTPTYPE_ASCII) { - *ptr++ = '\r'; - size++; - } - - *ptr++ = ch; - size++; - - /* flush if necessary */ - if (FTP_BUFSIZE - size < 2) { - if (my_send(ftp, ftp->data->fd, ftp->data->buf, size) != size) { - goto bail; - } - return PHP_FTP_MOREDATA; - } - } - - if (size && my_send(ftp, ftp->data->fd, ftp->data->buf, size) != size) { + if (ftp_send_stream_to_data_socket(ftp, ftp->data, ftp->stream, ftp->type, true) != SUCCESS) { goto bail; } + + if (!php_stream_eof(ftp->stream)) { + return PHP_FTP_MOREDATA; + } + data_close(ftp); if (!ftp_getresp(ftp) || (ftp->resp != 226 && ftp->resp != 250)) { diff --git a/ext/ftp/tests/ftp_fput_ascii_over_4_kib.phpt b/ext/ftp/tests/ftp_fput_ascii_over_4_kib.phpt new file mode 100644 index 00000000000..281332c2238 --- /dev/null +++ b/ext/ftp/tests/ftp_fput_ascii_over_4_kib.phpt @@ -0,0 +1,33 @@ +--TEST-- +Testing ftp_fput basic functionality +--EXTENSIONS-- +ftp +pcntl +--FILE-- + +--CLEAN-- + +--EXPECT-- +bool(true) +bool(true) +bool(true) +bool(true)