php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #47487 [PATCH]php_stream_fill_read_buffer() performance degraded after fix of bug 44607
Submitted: 2009-02-24 08:52 UTC Modified: 2009-03-23 16:12 UTC
From: basant dot kukreja at gmail dot com Assigned: lbarnaud
Status: Closed Package: Streams related
PHP Version: 5.2.9RC3 OS: Solaris 10
Private report: No CVE-ID:
 [2009-02-24 08:52 UTC] basant dot kukreja at gmail dot com
Description:
------------
After bug 44607 is fixed, php_stream_fill_read_buffer does many reallocs for
medium to large file (e.g 60KB)

Before the bug 44607 was fixed, php_stream_fill_read_buffer returns after
single read so there was no reallocs. But after the fix, it keeps reading
until the entire file is read.



Reproduce code:
---------------
If we add a printf in php_stream_fill_read_buffer function like :

       while (stream->writepos - stream->readpos < (off_t)size) {
            size_t justread = 0; 
            size_t toread;

            /* grow the buffer if required
             * TODO: this can fail for persistent streams */
            if (stream->readbuflen - stream->writepos < stream->chunk_size) {
                stream->readbuflen += stream->chunk_size;
                fprintf(stderr, "php_stream_fill_read_buffer :reallocating %d\n", 
                        stream->readbuflen);
                stream->readbuf = perealloc(stream->readbuf, stream->readbuflen,
                        stream->is_persistent);
            }

Then for a php script which includes 60KB html file, following
is printed.
stderr: php_stream_fill_read_buffer :reallocating 8192
stderr: php_stream_fill_read_buffer :reallocating 16384
stderr: php_stream_fill_read_buffer :reallocating 24576
stderr: php_stream_fill_read_buffer :reallocating 32768
stderr: php_stream_fill_read_buffer :reallocating 40960
stderr: php_stream_fill_read_buffer :reallocating 49152
stderr: php_stream_fill_read_buffer :reallocating 57344


Expected result:
----------------
Less number of memcpy. Because of the reallocs, huge amount of memcpy
happens. This happened after php-5.2.6.
http://bugs.php.net/bug.php?id=44607



Actual result:
--------------
Degrade in performance.

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2009-02-24 08:59 UTC] basant dot kukreja at gmail dot com
There might be several possible way to fix it. Here is one alternative which requires more memory but avoid includes :

@@ -1251,6 +1253,11 @@
         * 2K).  */
        if (php_stream_stat(src, &ssbuf) == 0 && ssbuf.sb.st_size > 0) {
                max_len = ssbuf.sb.st_size + step;
+               if (max_len > src->readbuflen) {
+                       src->readbuflen = max_len;
+                       src->readbuf = perealloc(src->readbuf, src->readbuflen,
+                                                               src->is_persistent);
+               }
        } else {
                max_len = step;
        }
--------------------------------
The above patch though have single alloc but still require large
memory allocation. Before this patch too, large memory is allocated
but by several folds.

Before PR http://bugs.php.net/bug.php?id=44607
was fixed, php only required 8KB memory at a time and no reallocs
happened.
 [2009-02-24 09:10 UTC] basant dot kukreja at gmail dot com
Sorry, file name was missing from the patch :

--- a/php-5.2.9RC3/main/streams/streams.c       Sun Feb 22 19:57:30 2009 -0800
+++ b/php-5.2.9RC3/main/streams/streams.c       Tue Feb 24 00:50:21 2009 -0800
@@ -1251,6 +1253,11 @@
         * 2K).  */
        if (php_stream_stat(src, &ssbuf) == 0 && ssbuf.sb.st_size > 0) {
                max_len = ssbuf.sb.st_size + step;
+               if (max_len > src->readbuflen) {
+                       src->readbuflen = max_len;
+                       src->readbuf = perealloc(src->readbuf, src->readbuflen,
+                                                               src->is_persistent);
+               }
        } else {
                max_len = step;
        }
 [2009-02-24 15:39 UTC] johannes@php.net
Arnaud, you fixed the other issue, can you please take a look at this regression. Thanks.
 [2009-02-24 17:10 UTC] lbarnaud@php.net
Do you mean that you experience this problem when including files ? I believe the parser/scanner reads by chunks of 8K. Can you please provide a reproduce script (with the large file) ?

The fix for #44607 fixed php_stream_fill_read_buffer() so that it fills the buffer with "size" bytes, as it is expected to do, and as it was already doing on filtered streams.

If you call fread() with a size of 60K, php_stream_fill_read_buffer() will return 60K. Before the fix, it returned 8K, and php_stream_read() had to call it again and to do memcpy()s too.
 [2009-02-24 20:18 UTC] basant dot kukreja at gmail dot com
Here is the script :

testinc.php :
<?php
print "Hello\n";
file_get_contents("/tmp/" . 'index');
?>

$ mkfile 60k /tmp/index
$ ./sapi/cli/php /tmp/testinc.php 
Hello
php_stream_fill_read_buffer :reallocating 8192
php_stream_fill_read_buffer :reallocating 16384
php_stream_fill_read_buffer :reallocating 24576
php_stream_fill_read_buffer :reallocating 32768
php_stream_fill_read_buffer :reallocating 40960
php_stream_fill_read_buffer :reallocating 49152
php_stream_fill_read_buffer :reallocating 57344

-----------------------------------------------

printfs are coming from my fprintf addition in php_stream_fill_read_buffer:
    fprintf(stderr, "php_stream_fill_read_buffer :reallocating %d\n", 
            stream->readbuflen);
-----------------------------------------------
 [2009-02-25 12:26 UTC] lbarnaud@php.net
Thanks.

The following patch reverts the performance penalty introduced in the fix of #44607 :

--- main/streams/streams.c	8 Jan 2009 19:21:25 -0000	1.82.2.6.2.33
+++ main/streams/streams.c	25 Feb 2009 12:09:01 -0000
@@ -597,7 +597,7 @@ PHPAPI size_t _php_stream_read(php_strea
 		if (!stream->readfilters.head && (stream->flags & PHP_STREAM_FLAG_NO_BUFFER || stream->chunk_size == 1)) {
 			toread = stream->ops->read(stream, buf, size TSRMLS_CC);
 		} else {
-			php_stream_fill_read_buffer(stream, size TSRMLS_CC);
+			php_stream_fill_read_buffer(stream, MIN(size, stream->chunk_size) TSRMLS_CC);
 
 			toread = stream->writepos - stream->readpos;
 			if (toread > size) {


I will do more tests on this and commit after 5.2.9 is out.
 [2009-03-23 16:12 UTC] lbarnaud@php.net
This bug has been fixed in CVS.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.
 
Thank you for the report, and for helping us make PHP better.


 
PHP Copyright © 2001-2014 The PHP Group
All rights reserved.
Last updated: Wed Apr 23 14:02:33 2014 UTC