Don't use chunking for stream writes

nikic · nikic · commit 5cbe5a538c92 · 2019-10-30T13:03:45.000+01:00
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.
diff --git a/ext/standard/tests/file/userstreams_006.phpt b/ext/standard/tests/file/userstreams_006.phpt
@@ -32,6 +32,5 @@ bool(true)
 option: 3, 2, 50
 int(-1)
 int(8192)
-size: 42
-size: 28
+size: 70
 int(70)
diff --git a/ext/standard/tests/streams/set_file_buffer.phpt b/ext/standard/tests/streams/set_file_buffer.phpt
@@ -43,5 +43,4 @@ option: %d, %d, %d
 int(%i)
 int(%d)
 size: %d
-size: %d
 int(%d)
diff --git a/ext/standard/tests/streams/stream_set_chunk_size.phpt b/ext/standard/tests/streams/stream_set_chunk_size.phpt
@@ -34,7 +34,7 @@ echo "should return previous chunk size (8192)\n";
 var_dump(stream_set_chunk_size($f, 1));
 echo "should be read without buffer (\$count == 10000)\n";
 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)));
 
 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)));
 echo "should elicit no read because there is sufficient cached data\n";
 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))));
 
 echo "\nerror conditions\n";
@@ -58,10 +58,8 @@ int(8192)
 should be read without buffer ($count == 10000)
 read with size: 10000
 int(10000)
-should elicit 3 writes of size 1 and return 3
-write with size: 1
-write with size: 1
-write with size: 1
+should have no effect on writes
+write with size: 3
 int(3)
 should return previous chunk size (1)
 int(1)
@@ -73,10 +71,8 @@ read with size: 100
 int(50)
 should elicit no read because there is sufficient cached data
 int(50)
-should elicit 2 writes of size 100 and one of size 50
-write with size: 100
-write with size: 100
-write with size: 50
+should have no effect on writes
+write with size: 250
 int(3)
 
 error conditions
diff --git a/main/streams/streams.c b/main/streams/streams.c
@@ -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 */
 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
  	 * 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);
 	}
 
-
 	while (count > 0) {
-		size_t towrite = count;
-		if (towrite > stream->chunk_size)
-			towrite = stream->chunk_size;
-
-		justwrote = stream->ops->write(stream, buf, towrite);
+		ssize_t justwrote = stream->ops->write(stream, buf, count);
 		if (justwrote <= 0) {
 			/* If we already successfully wrote some bytes and a write error occurred
 			 * later, report the successfully written bytes. */
