Don't use chunking for stream writes

We're currently splitting up large writes into 8K size chunks, which
adversely affects I/O performance in some cases. Splitting up writes
doesn't make a lot of sense, as we already must have a backing buffer,
so there is no memory/performance tradeoff to be made here.

This change disables the write chunking at the stream layer, but
retains the current retry loop for partial writes. In particular
network writes will typically only write part of the data for large
writes, so we need to keep the retry loop to preserve backwards
compatibility.

If issues due to this change turn up, chunking should be reintroduced
at lower levels where it is needed to avoid issues for specific streams,
rather than unnecessarily enforcing it for all streams.
This commit is contained in:
Nikita Popov 2019-07-18 17:02:29 +02:00
parent f9ab339c0f
commit 5cbe5a538c
4 changed files with 9 additions and 20 deletions

View file

@ -32,6 +32,5 @@ bool(true)
option: 3, 2, 50 option: 3, 2, 50
int(-1) int(-1)
int(8192) int(8192)
size: 42 size: 70
size: 28
int(70) int(70)

View file

@ -43,5 +43,4 @@ option: %d, %d, %d
int(%i) int(%i)
int(%d) int(%d)
size: %d size: %d
size: %d
int(%d) int(%d)

View file

@ -34,7 +34,7 @@ echo "should return previous chunk size (8192)\n";
var_dump(stream_set_chunk_size($f, 1)); var_dump(stream_set_chunk_size($f, 1));
echo "should be read without buffer (\$count == 10000)\n"; echo "should be read without buffer (\$count == 10000)\n";
var_dump(strlen(fread($f, 10000))); var_dump(strlen(fread($f, 10000)));
echo "should elicit 3 writes of size 1 and return 3\n"; echo "should have no effect on writes\n";
var_dump(fwrite($f, str_repeat('b', 3))); var_dump(fwrite($f, str_repeat('b', 3)));
echo "should return previous chunk size (1)\n"; echo "should return previous chunk size (1)\n";
@ -45,7 +45,7 @@ echo "should elicit one read of size 100 (chunk size)\n";
var_dump(strlen(fread($f, 50))); var_dump(strlen(fread($f, 50)));
echo "should elicit no read because there is sufficient cached data\n"; echo "should elicit no read because there is sufficient cached data\n";
var_dump(strlen(fread($f, 50))); var_dump(strlen(fread($f, 50)));
echo "should elicit 2 writes of size 100 and one of size 50\n"; echo "should have no effect on writes\n";
var_dump(strlen(fwrite($f, str_repeat('b', 250)))); var_dump(strlen(fwrite($f, str_repeat('b', 250))));
echo "\nerror conditions\n"; echo "\nerror conditions\n";
@ -58,10 +58,8 @@ int(8192)
should be read without buffer ($count == 10000) should be read without buffer ($count == 10000)
read with size: 10000 read with size: 10000
int(10000) int(10000)
should elicit 3 writes of size 1 and return 3 should have no effect on writes
write with size: 1 write with size: 3
write with size: 1
write with size: 1
int(3) int(3)
should return previous chunk size (1) should return previous chunk size (1)
int(1) int(1)
@ -73,10 +71,8 @@ read with size: 100
int(50) int(50)
should elicit no read because there is sufficient cached data should elicit no read because there is sufficient cached data
int(50) int(50)
should elicit 2 writes of size 100 and one of size 50 should have no effect on writes
write with size: 100 write with size: 250
write with size: 100
write with size: 50
int(3) int(3)
error conditions error conditions

View file

@ -1105,7 +1105,7 @@ PHPAPI zend_string *php_stream_get_record(php_stream *stream, size_t maxlen, con
/* Writes a buffer directly to a stream, using multiple of the chunk size */ /* Writes a buffer directly to a stream, using multiple of the chunk size */
static ssize_t _php_stream_write_buffer(php_stream *stream, const char *buf, size_t count) static ssize_t _php_stream_write_buffer(php_stream *stream, const char *buf, size_t count)
{ {
ssize_t didwrite = 0, justwrote; ssize_t didwrite = 0;
/* if we have a seekable stream we need to ensure that data is written at the /* if we have a seekable stream we need to ensure that data is written at the
* current stream->position. This means invalidating the read buffer and then * current stream->position. This means invalidating the read buffer and then
@ -1116,13 +1116,8 @@ static ssize_t _php_stream_write_buffer(php_stream *stream, const char *buf, siz
stream->ops->seek(stream, stream->position, SEEK_SET, &stream->position); stream->ops->seek(stream, stream->position, SEEK_SET, &stream->position);
} }
while (count > 0) { while (count > 0) {
size_t towrite = count; ssize_t justwrote = stream->ops->write(stream, buf, count);
if (towrite > stream->chunk_size)
towrite = stream->chunk_size;
justwrote = stream->ops->write(stream, buf, towrite);
if (justwrote <= 0) { if (justwrote <= 0) {
/* If we already successfully wrote some bytes and a write error occurred /* If we already successfully wrote some bytes and a write error occurred
* later, report the successfully written bytes. */ * later, report the successfully written bytes. */