FAQ
Hi,

the pull request https://github.com/php/php-src/pull/500 fixing the bug
#50333 is ready to review. Manual tests done so far on linux and windows
in TS and NTS mode with CLI and Apache show no regression. The performance
tests are to be done yet.

Regards

Anatol

Search Discussions

  • Dmitry Stogov at Oct 21, 2013 at 12:30 pm
    Hi,

    I don't have strong opinion about the patch.
    I thought malloc() -> emalloc() change might improve PHP performance in
    general, but unfortunately it doesn't.
    On the other hand the patch is quite big and introduces source level
    incompatibility.

    The patch is not complete. At least it misses this chunk:

    --- a/sapi/cgi/cgi_main.c
    +++ b/sapi/cgi/cgi_main.c
    @@ -1396,7 +1396,7 @@ static void init_request_info(fcgi_request *request
    TSRMLS_DC)
                                     } else {
                                             SG(request_info).request_uri =
    env_script_name;
                                     }
    - free(real_path);
    + efree(real_path);
                             }
                     } else {
                             /* pre 4.3 behaviour, shouldn't be used but
    provides BC */

    It's also much better to use do_alloca() instead of tsrm_do_alloca() (the
    patch changed them to less efficient emalloc()).
    May be if you change it, we would see improvement :)

    As I said, currently, the patch doesn't significantly affect performance of
    non-thread-safe build on Linux.


    master patched improvement Blog (req/sec) 106.1 105.1 -0.94% drupal
    (req/sec) 1660.5 1668.6 0.49% fw (req/sec) 231.7 227.499 -1.81% hello
    (req/sec) 11828.8 11980.5 1.28% qdig (req/sec) 470 477.3 1.55% typo3
    (req/sec) 580.1 579.3 -0.14% wordpress (req/sec) 185.9 188.5 1.40% xoops
    (req/sec) 130 131.2 0.92% scrum (req/sec) 185.199 185 -0.11% ZF1 Hello
    (req/sec) 1154.7 1155.2 0.04% ZF2 Test (req/sec) 248.7 250.7 0.80%
    Thanks. Dmitry.


    On Fri, Oct 18, 2013 at 7:33 PM, Anatol Belski wrote:

    Hi,

    the pull request https://github.com/php/php-src/pull/500 fixing the bug
    #50333 is ready to review. Manual tests done so far on linux and windows
    in TS and NTS mode with CLI and Apache show no regression. The performance
    tests are to be done yet.

    Regards

    Anatol
  • Anatol Belski at Oct 23, 2013 at 10:55 am
    Hi Dmitry,
    On Mon, October 21, 2013 14:30, Dmitry Stogov wrote:
    Hi,


    I don't have strong opinion about the patch.
    I thought malloc() -> emalloc() change might improve PHP performance in
    general, but unfortunately it doesn't. On the other hand the patch is quite
    big and introduces source level incompatibility.

    The patch is not complete. At least it misses this chunk:


    --- a/sapi/cgi/cgi_main.c
    +++ b/sapi/cgi/cgi_main.c
    @@ -1396,7 +1396,7 @@ static void init_request_info(fcgi_request *request
    TSRMLS_DC)
    } else {
    SG(request_info).request_uri =
    env_script_name; }
    - free(real_path);
    + efree(real_path);
    }
    } else {
    /* pre 4.3 behaviour, shouldn't be used but
    provides BC */

    It's also much better to use do_alloca() instead of tsrm_do_alloca() (the
    patch changed them to less efficient emalloc()). May be if you change it,
    we would see improvement :)

    As I said, currently, the patch doesn't significantly affect performance
    of non-thread-safe build on Linux.


    master patched improvement Blog (req/sec) 106.1 105.1 -0.94% drupal
    (req/sec) 1660.5 1668.6 0.49% fw (req/sec) 231.7 227.499 -1.81% hello
    (req/sec) 11828.8 11980.5 1.28% qdig (req/sec) 470 477.3 1.55% typo3
    (req/sec) 580.1 579.3 -0.14% wordpress (req/sec) 185.9 188.5 1.40% xoops
    (req/sec) 130 131.2 0.92% scrum (req/sec) 185.199 185 -0.11% ZF1 Hello
    (req/sec) 1154.7 1155.2 0.04% ZF2 Test (req/sec) 248.7 250.7 0.80%
    Thanks. Dmitry.




    On Fri, Oct 18, 2013 at 7:33 PM, Anatol Belski wrote:

    Hi,


    the pull request https://github.com/php/php-src/pull/500 fixing the bug
    #50333 is ready to review. Manual tests done so far on linux and
    windows in TS and NTS mode with CLI and Apache show no regression. The
    performance tests are to be done yet.

    Regards


    Anatol
    thanks for the comments, the CGI part is fixed in the same patch.

    While investigating on how to go further with your suggestion, I've yet a
    couple of open questions.

    The reason for moving files into Zend, as I can see from the original
    patch is, that the tsrm_init has to happen after Zend memory manager init
    in order to use the e* function family. It has to be true also for the
    do_alloca() case as when the stack size was exceeded, it'll fallback to
    emalloc(). For that reason, I'd rather implement your suggestion into the
    existing patch as it's quicker just to see if it'd make sense at all. If
    it would, tsrm files can be moved back into TSRM/ and one can look for
    magic to initialize it in the right order.

    It'd of course make sense to convert everything for do_alloca() usage,
    just a replace for tsrm_do_alloca would already work, but ... there are
    some places in the original codelike
    http://lxr.php.net/xref/PHP_TRUNK/TSRM/tsrm_virtual_cwd.c#493 where malloc
    is used. Or virtual_file_ex which is using realloc(), with do_alloca that
    memory should be probably just left unfreed on the stack? Do you generally
    think i should touch those places, would it be ok to leave them alone for
    now for the tests? As otherwise it might need signature change of some
    functions, it's about use_heap when the stack size is exceeded to avoid
    memory leaks.

    When I look here http://lxr.php.net/xref/PHP_TRUNK/Zend/zend.h#200 , only
    some GNU based platform profits from alloca() on both TS and NTS. Some
    others are only active with NTS only. Is there a particular reason for
    that? I think that macros could be refactored at least for the Windows
    part, as this page
    http://msdn.microsoft.com/en-us/library/5471dc8s(v=vs.110).aspx deprecates
    _alloca nowadays.

    Also wondering why you was doing NTS perf tests, as the ticket is about
    improvement for multi-threaded envs. Nevertheless the overall improvement
    were even better of course :)

    Regards

    Anatol
  • Dmitry Stogov at Oct 23, 2013 at 1:56 pm
    Hi Anatol,

    First of all PHP is mainly used as NTS on Linux (LAMP stack)
    It's the reason I tested it first, to check if your patch affects the
    performance that is really important for users.

    I still think that tsrm_do_alloca() have to be replaced by do_alloca().
    They have exactly the same logic, except that the first one fallback to
    malloc() and the second one to emalloc(). It must not be affected by
    malloc()s and realloc()s if you changed them into emalloc() and erealloc().
    May be I missing something...

    I'm not sure about alloca() usage on Windows, but you may change zend.h if
    you like.
    BTW: I don't think we should replace alloca() by _malloca(), because
    do_alloca() already does the same in portable way and uses per-request heap.

    Thanks. Dmitry.

    On Wed, Oct 23, 2013 at 2:55 PM, Anatol Belski wrote:

    Hi Dmitry,
    On Mon, October 21, 2013 14:30, Dmitry Stogov wrote:
    Hi,


    I don't have strong opinion about the patch.
    I thought malloc() -> emalloc() change might improve PHP performance in
    general, but unfortunately it doesn't. On the other hand the patch is quite
    big and introduces source level incompatibility.

    The patch is not complete. At least it misses this chunk:


    --- a/sapi/cgi/cgi_main.c
    +++ b/sapi/cgi/cgi_main.c
    @@ -1396,7 +1396,7 @@ static void init_request_info(fcgi_request *request
    TSRMLS_DC)
    } else {
    SG(request_info).request_uri =
    env_script_name; }
    - free(real_path);
    + efree(real_path);
    }
    } else {
    /* pre 4.3 behaviour, shouldn't be used but
    provides BC */

    It's also much better to use do_alloca() instead of tsrm_do_alloca() (the
    patch changed them to less efficient emalloc()). May be if you change it,
    we would see improvement :)

    As I said, currently, the patch doesn't significantly affect performance
    of non-thread-safe build on Linux.


    master patched improvement Blog (req/sec) 106.1 105.1 -0.94% drupal
    (req/sec) 1660.5 1668.6 0.49% fw (req/sec) 231.7 227.499 -1.81% hello
    (req/sec) 11828.8 11980.5 1.28% qdig (req/sec) 470 477.3 1.55% typo3
    (req/sec) 580.1 579.3 -0.14% wordpress (req/sec) 185.9 188.5 1.40% xoops
    (req/sec) 130 131.2 0.92% scrum (req/sec) 185.199 185 -0.11% ZF1 Hello
    (req/sec) 1154.7 1155.2 0.04% ZF2 Test (req/sec) 248.7 250.7 0.80%
    Thanks. Dmitry.




    On Fri, Oct 18, 2013 at 7:33 PM, Anatol Belski wrote:

    Hi,


    the pull request https://github.com/php/php-src/pull/500 fixing the bug
    #50333 is ready to review. Manual tests done so far on linux and
    windows in TS and NTS mode with CLI and Apache show no regression. The
    performance tests are to be done yet.

    Regards


    Anatol
    thanks for the comments, the CGI part is fixed in the same patch.

    While investigating on how to go further with your suggestion, I've yet a
    couple of open questions.

    The reason for moving files into Zend, as I can see from the original
    patch is, that the tsrm_init has to happen after Zend memory manager init
    in order to use the e* function family. It has to be true also for the
    do_alloca() case as when the stack size was exceeded, it'll fallback to
    emalloc(). For that reason, I'd rather implement your suggestion into the
    existing patch as it's quicker just to see if it'd make sense at all. If
    it would, tsrm files can be moved back into TSRM/ and one can look for
    magic to initialize it in the right order.

    It'd of course make sense to convert everything for do_alloca() usage,
    just a replace for tsrm_do_alloca would already work, but ... there are
    some places in the original codelike
    http://lxr.php.net/xref/PHP_TRUNK/TSRM/tsrm_virtual_cwd.c#493 where malloc
    is used. Or virtual_file_ex which is using realloc(), with do_alloca that
    memory should be probably just left unfreed on the stack? Do you generally
    think i should touch those places, would it be ok to leave them alone for
    now for the tests? As otherwise it might need signature change of some
    functions, it's about use_heap when the stack size is exceeded to avoid
    memory leaks.

    When I look here http://lxr.php.net/xref/PHP_TRUNK/Zend/zend.h#200 , only
    some GNU based platform profits from alloca() on both TS and NTS. Some
    others are only active with NTS only. Is there a particular reason for
    that? I think that macros could be refactored at least for the Windows
    part, as this page
    http://msdn.microsoft.com/en-us/library/5471dc8s(v=vs.110).aspx deprecates
    _alloca nowadays.

    Also wondering why you was doing NTS perf tests, as the ticket is about
    improvement for multi-threaded envs. Nevertheless the overall improvement
    were even better of course :)

    Regards

    Anatol
  • Anatol Belski at Oct 28, 2013 at 6:31 pm
    Hi Dmitry,
    On Wed, October 23, 2013 14:55, Dmitry Stogov wrote:
    Hi Anatol,


    First of all PHP is mainly used as NTS on Linux (LAMP stack)
    It's the reason I tested it first, to check if your patch affects the
    performance that is really important for users.

    I still think that tsrm_do_alloca() have to be replaced by do_alloca().
    They have exactly the same logic, except that the first one fallback to
    malloc() and the second one to emalloc(). It must not be affected by
    malloc()s and realloc()s if you changed them into emalloc() and
    erealloc(). May be I missing something...


    I'm not sure about alloca() usage on Windows, but you may change zend.h
    if you like. BTW: I don't think we should replace alloca() by _malloca(),
    because do_alloca() already does the same in portable way and uses
    per-request heap.
    last week I've updated the patch according to your recommendations, now we
    have the performance test results

    http://windows.php.net/downloads/snaps/ostc/pftt/perf/results-20131025-MasterVanilla-Master50333-3820.html
    http://windows.php.net/downloads/snaps/ostc/pftt/perf/results-20131025-MasterVanilla-Master50333-7795.html

    Also enabled the stack usage in both TS/NTS for windows. It shows still no
    improvement as one can see. Though, there are some functions, like
    virtual_file_ex(), which use emalloc() only in the patch, but called very
    often within the virtual cwd API. Changing them would require signature
    changes os one can efree() in case of the fallback situation, but still
    it's doubtful that would tell some bigger difference in performance. Do
    you think it's worth a try?

    Regards

    Anatol
  • Dmitry Stogov at Oct 29, 2013 at 7:45 am
    Hi Anatol,

    Thank you for update.
    I'm surprised you don't see speed difference even with ZTS :(

    I didn't get what you propose to change in virtual_file_ex().

    I'll do a more careful patch review later on this week.

    Thaks. Dmitry.

    On Mon, Oct 28, 2013 at 10:31 PM, Anatol Belski wrote:

    Hi Dmitry,
    On Wed, October 23, 2013 14:55, Dmitry Stogov wrote:
    Hi Anatol,


    First of all PHP is mainly used as NTS on Linux (LAMP stack)
    It's the reason I tested it first, to check if your patch affects the
    performance that is really important for users.

    I still think that tsrm_do_alloca() have to be replaced by do_alloca().
    They have exactly the same logic, except that the first one fallback to
    malloc() and the second one to emalloc(). It must not be affected by
    malloc()s and realloc()s if you changed them into emalloc() and
    erealloc(). May be I missing something...


    I'm not sure about alloca() usage on Windows, but you may change zend.h
    if you like. BTW: I don't think we should replace alloca() by _malloca(),
    because do_alloca() already does the same in portable way and uses
    per-request heap.
    last week I've updated the patch according to your recommendations, now we
    have the performance test results


    http://windows.php.net/downloads/snaps/ostc/pftt/perf/results-20131025-MasterVanilla-Master50333-3820.html

    http://windows.php.net/downloads/snaps/ostc/pftt/perf/results-20131025-MasterVanilla-Master50333-7795.html

    Also enabled the stack usage in both TS/NTS for windows. It shows still no
    improvement as one can see. Though, there are some functions, like
    virtual_file_ex(), which use emalloc() only in the patch, but called very
    often within the virtual cwd API. Changing them would require signature
    changes os one can efree() in case of the fallback situation, but still
    it's doubtful that would tell some bigger difference in performance. Do
    you think it's worth a try?

    Regards

    Anatol
  • Anatol Belski at Oct 30, 2013 at 2:25 am
    Hi Dmitry,
    On Tue, 2013-10-29 at 11:45 +0400, Dmitry Stogov wrote:
    Hi Anatol,


    Thank you for update.

    I'm surprised you don't see speed difference even with ZTS :(


    I didn't get what you propose to change in virtual_file_ex().


    I'll do a more careful patch review later on this week.
    the story is that

    - the original patch had replacements for all tsrm_do_alloca() and
    malloc() to emaloc()
    - after that, i've turned the places originally having tsrm_do_alloca()
    to do_alloca()

    Just as we discussed, and that's the current state of the patch. Still,
    there are many places using the e*() family of functions, like

    https://github.com/weltling/php-src/blob/bug50333/Zend/zend_virtual_cwd.c#L151
    https://github.com/weltling/php-src/blob/bug50333/Zend/zend_virtual_cwd.c#L1340

    Alone for that two I find 21 occurrences of each elsewhere in the file.
    As they are used very often in other filesystem functions like
    virtual_copy(), virtual_rename(), etc. That's why I thought it could
    make sense to turn any emalloc() usage into do_alloca(). But as the
    do_alloca(size, use_heap) requires that use_heap argument to determine
    the fallback situation, the signatures in at least that two cases for
    virtual_file_ex() or CWD_STATE_COPY() will need to be extended, so the
    correct freeing can happen. A simple snippet in pseudo code, just to
    illustrate the idea

    currently:

    some_function()
    {
    CWD_STATE_COPY(&new_state, &CWDG(cwd));
    virtual_file_ex(&new_state, path, NULL, CWD_REALPATH TSRMLS_CC);
    CWD_STATE_FREE(&new_state);
    }

    should be like:

    some_function()
    {
    ALLOCA_FLAG(use_heap)
    CWD_STATE_COPY(&new_state, &CWDG(cwd), use_heap);
    virtual_file_ex(&new_state, path, NULL, CWD_REALPATH, use_heap
    TSRMLS_CC);
    CWD_STATE_FREE(&new_state, use_heap);
    }

    That's of course much deeper change and would affect more code outside
    just the scope of that one file, still that's manageable. I'd expect
    some more functions needing that, so then we'd have do_alloca() usage
    everywhere and drop all e*() invocations. Maybe then we see some
    effect :)

    Regards

    Anatol
  • Dmitry Stogov at Oct 30, 2013 at 6:49 am
    I don't think it makes sense to invest into it right now.
    Lets finish with existing patch.

    Also, alloca() allocates data on CPU stack, so such data is automatically
    freed when function returns.
    In case you change CWD_STATE_COPY() to use alloca(), the "state" couldn't
    be returned from the function.
    I'm not sure if it'll work for all use cases.

    Thanks. Dmitry.

    On Wed, Oct 30, 2013 at 6:25 AM, Anatol Belski wrote:

    Hi Dmitry,
    On Tue, 2013-10-29 at 11:45 +0400, Dmitry Stogov wrote:
    Hi Anatol,


    Thank you for update.

    I'm surprised you don't see speed difference even with ZTS :(


    I didn't get what you propose to change in virtual_file_ex().


    I'll do a more careful patch review later on this week.
    the story is that

    - the original patch had replacements for all tsrm_do_alloca() and
    malloc() to emaloc()
    - after that, i've turned the places originally having tsrm_do_alloca()
    to do_alloca()

    Just as we discussed, and that's the current state of the patch. Still,
    there are many places using the e*() family of functions, like


    https://github.com/weltling/php-src/blob/bug50333/Zend/zend_virtual_cwd.c#L151

    https://github.com/weltling/php-src/blob/bug50333/Zend/zend_virtual_cwd.c#L1340

    Alone for that two I find 21 occurrences of each elsewhere in the file.
    As they are used very often in other filesystem functions like
    virtual_copy(), virtual_rename(), etc. That's why I thought it could
    make sense to turn any emalloc() usage into do_alloca(). But as the
    do_alloca(size, use_heap) requires that use_heap argument to determine
    the fallback situation, the signatures in at least that two cases for
    virtual_file_ex() or CWD_STATE_COPY() will need to be extended, so the
    correct freeing can happen. A simple snippet in pseudo code, just to
    illustrate the idea

    currently:

    some_function()
    {
    CWD_STATE_COPY(&new_state, &CWDG(cwd));
    virtual_file_ex(&new_state, path, NULL, CWD_REALPATH TSRMLS_CC);
    CWD_STATE_FREE(&new_state);
    }

    should be like:

    some_function()
    {
    ALLOCA_FLAG(use_heap)
    CWD_STATE_COPY(&new_state, &CWDG(cwd), use_heap);
    virtual_file_ex(&new_state, path, NULL, CWD_REALPATH, use_heap
    TSRMLS_CC);
    CWD_STATE_FREE(&new_state, use_heap);
    }

    That's of course much deeper change and would affect more code outside
    just the scope of that one file, still that's manageable. I'd expect
    some more functions needing that, so then we'd have do_alloca() usage
    everywhere and drop all e*() invocations. Maybe then we see some
    effect :)

    Regards

    Anatol




  • Dmitry Stogov at Oct 30, 2013 at 12:00 pm
    Hi Anatol,

    I've posted few comments at https://github.com/php/php-src/pull/500/files
    Otherwise, the patch looks fine.
    I think it may be accepted even if it doesn't make visible improvement.
    Of course, when the small issues described in comment are fixed.

    There were also a minor merging conflict in ext/opcache/ZendAccelerator.c
    (it's not a problem at all).

    I hope you testd ZTS PHP with patch well, because I mainly care about
    non-ZTS builds.

    Thanks. Dmitry.

    On Wed, Oct 30, 2013 at 10:49 AM, Dmitry Stogov wrote:

    I don't think it makes sense to invest into it right now.
    Lets finish with existing patch.

    Also, alloca() allocates data on CPU stack, so such data is automatically
    freed when function returns.
    In case you change CWD_STATE_COPY() to use alloca(), the "state" couldn't
    be returned from the function.
    I'm not sure if it'll work for all use cases.

    Thanks. Dmitry.

    On Wed, Oct 30, 2013 at 6:25 AM, Anatol Belski wrote:

    Hi Dmitry,
    On Tue, 2013-10-29 at 11:45 +0400, Dmitry Stogov wrote:
    Hi Anatol,


    Thank you for update.

    I'm surprised you don't see speed difference even with ZTS :(


    I didn't get what you propose to change in virtual_file_ex().


    I'll do a more careful patch review later on this week.
    the story is that

    - the original patch had replacements for all tsrm_do_alloca() and
    malloc() to emaloc()
    - after that, i've turned the places originally having tsrm_do_alloca()
    to do_alloca()

    Just as we discussed, and that's the current state of the patch. Still,
    there are many places using the e*() family of functions, like


    https://github.com/weltling/php-src/blob/bug50333/Zend/zend_virtual_cwd.c#L151

    https://github.com/weltling/php-src/blob/bug50333/Zend/zend_virtual_cwd.c#L1340

    Alone for that two I find 21 occurrences of each elsewhere in the file.
    As they are used very often in other filesystem functions like
    virtual_copy(), virtual_rename(), etc. That's why I thought it could
    make sense to turn any emalloc() usage into do_alloca(). But as the
    do_alloca(size, use_heap) requires that use_heap argument to determine
    the fallback situation, the signatures in at least that two cases for
    virtual_file_ex() or CWD_STATE_COPY() will need to be extended, so the
    correct freeing can happen. A simple snippet in pseudo code, just to
    illustrate the idea

    currently:

    some_function()
    {
    CWD_STATE_COPY(&new_state, &CWDG(cwd));
    virtual_file_ex(&new_state, path, NULL, CWD_REALPATH TSRMLS_CC);
    CWD_STATE_FREE(&new_state);
    }

    should be like:

    some_function()
    {
    ALLOCA_FLAG(use_heap)
    CWD_STATE_COPY(&new_state, &CWDG(cwd), use_heap);
    virtual_file_ex(&new_state, path, NULL, CWD_REALPATH, use_heap
    TSRMLS_CC);
    CWD_STATE_FREE(&new_state, use_heap);
    }

    That's of course much deeper change and would affect more code outside
    just the scope of that one file, still that's manageable. I'd expect
    some more functions needing that, so then we'd have do_alloca() usage
    everywhere and drop all e*() invocations. Maybe then we see some
    effect :)

    Regards

    Anatol




  • Anatol Belski at Oct 30, 2013 at 12:31 pm
    Hi Dmitry,
    On Wed, 2013-10-30 at 16:00 +0400, Dmitry Stogov wrote:
    Hi Anatol,

    I've posted few comments at https://github.com/php/php-src/pull/500/files
    Otherwise, the patch looks fine.
    I think it may be accepted even if it doesn't make visible improvement.
    Of course, when the small issues described in comment are fixed.

    There were also a minor merging conflict in ext/opcache/ZendAccelerator.c
    (it's not a problem at all).

    I hope you testd ZTS PHP with patch well, because I mainly care about
    non-ZTS builds.

    Thanks. Dmitry.
    thanks for the time you spend on this. The remaining fixes are on their
    way, they'll be tested well functionally+performance. I'll write back as
    soon as all that is done.

    What just comes in my mind, the original idea was invented on Solaris,
    so maybe there's something specific to that platform what makes it
    faster there. In the ticked is also mentioned "scalability on
    multi-threaded/multi-core systems", so maybe some more special
    conditions are needed. Maybe like some particular CPU cores count
    (currently we do it with 2 or 4 cores) or specific server load level.
    I'm going to play more with that.

    Best regards

    Anatol
  • Anatol Belski at Nov 2, 2013 at 12:45 pm
    Hi Dmitry,
    On Wed, October 30, 2013 13:00, Dmitry Stogov wrote:
    Hi Anatol,


    I've posted few comments at https://github.com/php/php-src/pull/500/files
    Otherwise, the patch looks fine.
    I think it may be accepted even if it doesn't make visible improvement.
    Of course, when the small issues described in comment are fixed.


    There were also a minor merging conflict in ext/opcache/ZendAccelerator.c
    (it's not a problem at all).


    I hope you testd ZTS PHP with patch well, because I mainly care about
    non-ZTS builds.

    Thanks. Dmitry.
    I've fixed the patch according to your last comments. Just a note about
    virtual_cwd_activate() being called twice. That is needed for the threaded
    environment, once for SAPI startup and then per request in
    zend_activate(), as that calls belong to different threads.

    The performance has at least no regressions, here's the report

    http://windows.php.net/downloads/snaps/ostc/pftt/perf/results-20131101-MasterVanilla-Master50333Patch2-2742.html

    The functional tests have also no regressions from recent master.

    Regards

    Anatol
  • Dmitry Stogov at Nov 5, 2013 at 6:47 am
    Hi Anatol,

    I don't see any technical problems with the patch.

    Thanks. Dmitry.


    On Sat, Nov 2, 2013 at 4:45 PM, Anatol Belski wrote:

    Hi Dmitry,
    On Wed, October 30, 2013 13:00, Dmitry Stogov wrote:
    Hi Anatol,


    I've posted few comments at
    https://github.com/php/php-src/pull/500/files
    Otherwise, the patch looks fine.
    I think it may be accepted even if it doesn't make visible improvement.
    Of course, when the small issues described in comment are fixed.


    There were also a minor merging conflict in ext/opcache/ZendAccelerator.c
    (it's not a problem at all).


    I hope you testd ZTS PHP with patch well, because I mainly care about
    non-ZTS builds.

    Thanks. Dmitry.
    I've fixed the patch according to your last comments. Just a note about
    virtual_cwd_activate() being called twice. That is needed for the threaded
    environment, once for SAPI startup and then per request in
    zend_activate(), as that calls belong to different threads.

    The performance has at least no regressions, here's the report


    http://windows.php.net/downloads/snaps/ostc/pftt/perf/results-20131101-MasterVanilla-Master50333Patch2-2742.html

    The functional tests have also no regressions from recent master.

    Regards

    Anatol

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedOct 18, '13 at 3:33p
activeNov 5, '13 at 6:47a
posts12
users2
websitephp.net

2 users in discussion

Dmitry Stogov: 6 posts Anatol Belski: 6 posts

People

Translate

site design / logo © 2022 Grokbase