Skip to content

Conversation

daxtens
Copy link
Contributor

@daxtens daxtens commented Sep 22, 2015

This does the same thing as 127db5a ("HANDLE_BLOCK_INTERRUPTIONS()
is not used by SAPIs anymore. It may be useful only when PHP configured
with --enable-zend-signals.")

In the PHP7 version, the code could be dropped easily because no SAPI
used the hooks. However, in PHP5, the Apache 1 SAPIs use these hooks.
Some other SAPIs provided NULL pointers.

With this patch, when PHP is compiled we first check for the Apache 1
SAPI, and if it is present, preserve the old behaviour. (Both flavours
of the Apache 1 SAPI define HAVE_APACHE, so use that.) It also preserves
the behaviour in the ZEND_SIGNALS case. Only if neither of those are
present do we compile out the definitions.

This leads to a speed up on PHP5 code. I've measured the performance
impact on a PowerPC 64-bit little-endian system and on a x86_64 laptop
using the following script:

I see a ~6% improvement in performance on POWER and a ~4% improvement
on x86_64.

I also see an improvement in a e-commerce web application on POWER
with this change.

Signed-off-by: Daniel Axtens dja@axtens.net


This is version 2.
Changes in this version from version 1 (#1520)

  • Do not change behaviour under Apache 1.

@daxtens
Copy link
Contributor Author

daxtens commented Sep 29, 2015

Pinging the original CC list: @dstogov @nikic @laruence @KalleZ

@dstogov
Copy link
Member

dstogov commented Sep 29, 2015

The patch doesn't respect zend-signal semantic with HAVE_APACHE.
It's not a big problem to change order of #ifdefs to achieve this.
In my opinion, this change should go only into major release, but I won't
object if RM would accept it.

On Tue, Sep 29, 2015 at 9:53 AM, Daniel Axtens notifications@github.com
wrote:

Pinging the original CC list: @dstogov https://github.com/dstogov @nikic
https://github.com/nikic @laruence https://github.com/laruence @KalleZ
https://github.com/KalleZ


Reply to this email directly or view it on GitHub
#1532 (comment).

@daxtens
Copy link
Contributor Author

daxtens commented Sep 29, 2015

Sorry, my mistake. I will update the patch tomorrow to reorder the ifdefs to keep the same semantic.

This does the same thing as 127db5a ("HANDLE_BLOCK_INTERRUPTIONS()
is not used by SAPIs anymore. It may be useful only when PHP configured
with --enable-zend-signals.")

In the PHP7 version, the code could be dropped easily because no SAPI
used the hooks. However, in PHP5, the Apache 1 SAPIs use these hooks.
Some other SAPIs provided NULL pointers.

With this patch, when PHP is compiled we first check for the Apache 1
SAPI, and if it is present, preserve the old behaviour. (Both flavours
of the Apache 1 SAPI define HAVE_APACHE, so use that.) It also preserves
the behaviour in the ZEND_SIGNALS case. Only if neither of those are
present do we compile out the definitions.

This leads to a speed up on PHP5 code. I've measured the performance
impact on a PowerPC 64-bit little-endian system and on a x86_64 laptop
using the following script:

<?php
for ($i=1; $i < 100000000; $i++) {
        $genericObject = new stdClass();
        $a = (object) 8;
        $b = (object) 'abcde';
}
?>

I see a ~6% improvement in performance on POWER and a ~4% improvement
on x86_64.

I also see an improvement in a e-commerce web application on POWER
with this change.

Signed-off-by: Daniel Axtens <dja@axtens.net>

---

This is version 3.
v2 -> v3:
 - Preserve ZEND_SIGNALS semantic.
v1 -> v2:
 - Do not change behaviour under Apache 1.
@daxtens
Copy link
Contributor Author

daxtens commented Sep 30, 2015

#ifdefs reordered as discussed.

Apologies for my ignorance of the PHP release process, but who is the 5.6 RM?

@dstogov
Copy link
Member

dstogov commented Sep 30, 2015

https://wiki.php.net/todo/php56

On Wed, Sep 30, 2015 at 9:35 AM, Daniel Axtens notifications@github.com
wrote:

#ifdefs reordered as discussed.

Apologies for my ignorance of the PHP release process, but who is the 5.6
RM?


Reply to this email directly or view it on GitHub
#1532 (comment).

@daxtens
Copy link
Contributor Author

daxtens commented Oct 14, 2015

Pinging @Tyrael and @jpauli as RMs: This speeds up PHP 5.6 and doesn't change functionality. Would it be possible to merge it please?

@Tyrael
Copy link
Contributor

Tyrael commented Oct 14, 2015

I don't think this worth changing in a micro release, there could be some other SAPIs (packaged by downstream) which depend on the existence of these macros.
@jpauli what do you think?

@jpauli
Copy link
Member

jpauli commented Oct 14, 2015

I agree with @Tyrael . Our 5.6 is stable, and we should not change it, but have changes go in PHP7 source base.

@daxtens
Copy link
Contributor Author

daxtens commented Oct 14, 2015

Well, would it be acceptable to include a change that doesn't change the
ifdefs but wraps the check in UNEXPECTED()? That should improve
performance in the common case, without breaking any downstream SAPI.

On Wed, Oct 14, 2015 at 11:21 PM, jpauli notifications@github.com wrote:

I agree with @Tyrael https://github.com/Tyrael . Our 5.6 is stable, and
we should not change it, but have changes go in PHP7 source base.


Reply to this email directly or view it on GitHub
#1532 (comment).

@dstogov
Copy link
Member

dstogov commented Oct 15, 2015

UNEXPECTED() will work.

On Thu, Oct 15, 2015 at 1:45 AM, Daniel Axtens notifications@github.com
wrote:

Well, would it be acceptable to include a change that doesn't change the
ifdefs but wraps the check in UNEXPECTED()? That should improve
performance in the common case, without breaking any downstream SAPI.

On Wed, Oct 14, 2015 at 11:21 PM, jpauli notifications@github.com wrote:

I agree with @Tyrael https://github.com/Tyrael . Our 5.6 is stable,
and
we should not change it, but have changes go in PHP7 source base.


Reply to this email directly or view it on GitHub
#1532 (comment).


Reply to this email directly or view it on GitHub
#1532 (comment).

@jpauli
Copy link
Member

jpauli commented Oct 15, 2015

UNEXPECTED makes sense

@daxtens
Copy link
Contributor Author

daxtens commented Oct 16, 2015

OK, I tried with UNEXPECTED().

Unfortunately that doesn't give a measurable performance benefit. I think this is because we still have to load the pointer to the globals struct, dereference it, load the function pointer from the struct and check the final pointer against NULL. Once we do all that UNEXPECTED makes us go faster, but it doesn't save us doing that work to begin with.

I'll continue to think about a non-intrusive way of doing this, but it looks like UNEXPECTED won't do it.

@nikic
Copy link
Member

nikic commented Mar 3, 2016

Closing this PR as the RM decision seems to be not to make this change in the 5.6 branch.

Also ref https://bugs.launchpad.net/ubuntu/+source/php5/+bug/1527105.

@nikic nikic closed this Mar 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants