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 (profile)
Status: Closed Package: Streams related
PHP Version: 5.2.9RC3 OS: Solaris 10
Private report: No CVE-ID: None
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: basant dot kukreja at gmail dot com
New email:
PHP Version: OS:

 

 [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

Pull Requests

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-2024 The PHP Group
All rights reserved.
Last updated: Sun Oct 27 16:01:27 2024 UTC