Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » The death of `#ifndef FAST_ZPP`? »

The death of `#ifndef FAST_ZPP`?

php.internals

Andrew Faulds

10 years ago
Hi everyone, You may recall that PHP 7.0 introduced a new API for processing arguments to internal functions, the "Fast Parameter Parsing API" or FAST_ZPP (https://wiki.php.net/rfc/fast_zpp), which is an alternative to the existing zend_parse_parameters function family. Since FAST_ZPP was implemented, many functions in built-in PHP extensions look like this: #ifndef FAST_ZPP if (zend_parse_parameters(ZEND_NUM_ARGS(), "S|S", &str, &what) == FAILURE) { return; } #else ZEND_PARSE_PARAMETERS_START(1, 2) Z_PARAM_STR(str) Z_PARAM_OPTIONAL Z_PARAM_STR(what) ZEND_PARSE_PARAMETERS_END(); #endif That is, they have /two/ sets of parameter processing code: one using the old API, and one using the new API, with conditional compilation. This is necessary because it is still possible to turn off FAST_ZPP by changing the value of the macro. However, should that be the case? The FAST_ZPP RFC was approved and PHP 7.0 was released containing it enabled by default, so it's no longer just an experimental performance enhancement. Furthermore, I have no reason to believe that the option to turn off FAST_ZPP is actually used in practice, though feel free to prove me wrong! Requiring all usages of FAST_ZPP to be accompanied by a zend_parse_parameters() fallback creates code bloat. It also creates hidden, potentially build-breaking bugs, because the code is rarely built with FAST_ZPP disabled. It's like the old TSRMLS_CC situation. Furthermore, for those who would prefer the FAST_ZPP API, it is inconvenient that they cannot use it exclusively and must also use zend_parse_parameters(), doubling the effort. So, I would like to ask: would there be any objections to going through and removing all unnecessary zend_parse_parameters fallbacks, and making it impossible to disable FAST_ZPP? This would make the two APIs equally valid. Thanks!
-- Andrea Faulds https://ajf.me/

Andrew Faulds

10 years ago
Hi everyone, 3 months and 6 days on, nobody has responded to this. Will these chunks of dead code ever be removed? Thanks! Andrea Faulds wrote:
> Hi everyone, > > You may recall that PHP 7.0 introduced a new API for processing > arguments to internal functions, the "Fast Parameter Parsing API" or > FAST_ZPP (https://wiki.php.net/rfc/fast_zpp), which is an alternative to > the existing zend_parse_parameters function family. > > Since FAST_ZPP was implemented, many functions in built-in PHP > extensions look like this: > > #ifndef FAST_ZPP > if (zend_parse_parameters(ZEND_NUM_ARGS(), "S|S", &str, &what) == > FAILURE) { > return; > } > #else > ZEND_PARSE_PARAMETERS_START(1, 2) > Z_PARAM_STR(str) > Z_PARAM_OPTIONAL > Z_PARAM_STR(what) > ZEND_PARSE_PARAMETERS_END(); > #endif > > That is, they have /two/ sets of parameter processing code: one using > the old API, and one using the new API, with conditional compilation. > This is necessary because it is still possible to turn off FAST_ZPP by > changing the value of the macro. > > However, should that be the case? The FAST_ZPP RFC was approved and PHP > 7.0 was released containing it enabled by default, so it's no longer > just an experimental performance enhancement. Furthermore, I have no > reason to believe that the option to turn off FAST_ZPP is actually used > in practice, though feel free to prove me wrong! > > Requiring all usages of FAST_ZPP to be accompanied by a > zend_parse_parameters() fallback creates code bloat. It also creates > hidden, potentially build-breaking bugs, because the code is rarely > built with FAST_ZPP disabled. It's like the old TSRMLS_CC situation. > > Furthermore, for those who would prefer the FAST_ZPP API, it is > inconvenient that they cannot use it exclusively and must also use > zend_parse_parameters(), doubling the effort. > > So, I would like to ask: would there be any objections to going through > and removing all unnecessary zend_parse_parameters fallbacks, and making > it impossible to disable FAST_ZPP? This would make the two APIs equally > valid. > > Thanks! >
-- Andrea Faulds https://ajf.me/

Levi Morrison

10 years ago
On Tue, Aug 9, 2016 at 8:23 PM, Andrea Faulds <ajf@ajf.me> wrote:
> Hi everyone, > > 3 months and 6 days on, nobody has responded to this. > > Will these chunks of dead code ever be removed? > > Thanks! > > > Andrea Faulds wrote: > >> Hi everyone, >> >> You may recall that PHP 7.0 introduced a new API for processing >> arguments to internal functions, the "Fast Parameter Parsing API" or >> FAST_ZPP (https://wiki.php.net/rfc/fast_zpp), which is an alternative to >> the existing zend_parse_parameters function family. >> >> Since FAST_ZPP was implemented, many functions in built-in PHP >> extensions look like this: >> >> #ifndef FAST_ZPP >> if (zend_parse_parameters(ZEND_NUM_ARGS(), "S|S", &str, &what) == >> FAILURE) { >> return; >> } >> #else >> ZEND_PARSE_PARAMETERS_START(1, 2) >> Z_PARAM_STR(str) >> Z_PARAM_OPTIONAL >> Z_PARAM_STR(what) >> ZEND_PARSE_PARAMETERS_END(); >> #endif >> >> That is, they have /two/ sets of parameter processing code: one using >> the old API, and one using the new API, with conditional compilation. >> This is necessary because it is still possible to turn off FAST_ZPP by >> changing the value of the macro. >> >> However, should that be the case? The FAST_ZPP RFC was approved and PHP >> 7.0 was released containing it enabled by default, so it's no longer >> just an experimental performance enhancement. Furthermore, I have no >> reason to believe that the option to turn off FAST_ZPP is actually used >> in practice, though feel free to prove me wrong! >> >> Requiring all usages of FAST_ZPP to be accompanied by a >> zend_parse_parameters() fallback creates code bloat. It also creates >> hidden, potentially build-breaking bugs, because the code is rarely >> built with FAST_ZPP disabled. It's like the old TSRMLS_CC situation. >> >> Furthermore, for those who would prefer the FAST_ZPP API, it is >> inconvenient that they cannot use it exclusively and must also use >> zend_parse_parameters(), doubling the effort. >> >> So, I would like to ask: would there be any objections to going through >> and removing all unnecessary zend_parse_parameters fallbacks, and making >> it impossible to disable FAST_ZPP? This would make the two APIs equally >> valid. >> >> Thanks! >> >> > > -- > Andrea Faulds > https://ajf.me/ > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php >
No objection from me.

Yasuo Ohgaki

10 years ago
Hi Andrea, On Wed, Aug 10, 2016 at 11:23 AM, Andrea Faulds <ajf@ajf.me> wrote:
> 3 months and 6 days on, nobody has responded to this. > > Will these chunks of dead code ever be removed?
I've asked the same question before I start eliminating old style parse API. Dimitry said it's designed to optimize heavily called functions, not for ease of use and the RFC for fast zpp states "keep old parameter parsing API", so I don't have to bother. +1 for enabling always (No #ifndef FAST_ZPP) and using FIRST_ZPP for core PHP functions. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Andrew Faulds

10 years ago
Hi Yasuo, Yasuo Ohgaki wrote:
> I've asked the same question before I start eliminating old style parse API. > > Dimitry said it's designed to optimize heavily called functions, not > for ease of use and the RFC for fast zpp states "keep old parameter > parsing API", so I don't have to bother.
Oh, yeah, I've heard about that. Even if it wasn't specifically intended to be a friendlier API, though, I've heard at least one other person say they prefer using FAST_ZPP, and I prefer it myself. It's more verbose, but it's easier to read at-a-glance, and it makes mistakes more obvious. Thanks.
-- Andrea Faulds https://ajf.me/

Fleshgrinder

10 years ago
On 8/10/2016 4:23 AM, Andrea Faulds wrote:
> Hi everyone, > > 3 months and 6 days on, nobody has responded to this. > > Will these chunks of dead code ever be removed? > > Thanks! > > Andrea Faulds wrote: >
+1 from my side, if it's faster it's faster. :)
-- Richard "Fleshgrinder" Fussenegger

Andrew Faulds

10 years ago
Hi again, I finally wrote a patch to get rid of it: https://github.com/php/php-src/pull/2118 Perhaps this will go somewhere. :)
-- Andrea Faulds https://ajf.me/

Ferenc Kovacs

9 years ago
On Tue, Sep 6, 2016 at 12:33 AM, Andrea Faulds <ajf@ajf.me> wrote:
> Hi again, > > I finally wrote a patch to get rid of it: > > https://github.com/php/php-src/pull/2118 > > Perhaps this will go somewhere. :) > > > -- > Andrea Faulds > https://ajf.me/ > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > >
+1 from me on the idea.
-- Ferenc Kovács @Tyr43l - http://tyrael.hu

Zeev Suraski

9 years ago
> -----Original Message----- > From: Ferenc Kovacs [mailto:tyra3l@gmail.com] > Sent: Wednesday, September 07, 2016 4:24 PM > To: Andrea Faulds <ajf@ajf.me> > Cc: PHP Internals <internals@lists.php.net> > Subject: Re: [PHP-DEV] Re: The death of `#ifndef FAST_ZPP`? > > On Tue, Sep 6, 2016 at 12:33 AM, Andrea Faulds <ajf@ajf.me> wrote: > > > Hi again, > > > > I finally wrote a patch to get rid of it: > > > > https://github.com/php/php-src/pull/2118 > > > > Perhaps this will go somewhere. :) > > > > > > -- > > Andrea Faulds > > https://ajf.me/ > > > > -- > > PHP Internals - PHP Runtime Development Mailing List To unsubscribe, > > visit: http://www.php.net/unsub.php > > > > > +1 from me on the idea.
+1 from me as well. Zeev

Andrew Faulds

9 years ago
Hi everyone, Andrea Faulds wrote:
> I finally wrote a patch to get rid of it: > > https://github.com/php/php-src/pull/2118 > > Perhaps this will go somewhere. :)
This has now been merged into PHP-7.0 and up: https://github.com/php/php-src/commit/d690014bf35507ccb7a1150a27504d2f87848842 Additionally, extra zpp fallback code only found in master has been removed here: https://github.com/php/php-src/commit/3cc90901010f016923e9ca6a4ae15ba08bc18169 Now there is no fallback zpp code around, and Fast ZPP is always enabled. The FAST_ZPP macro itself remains for compatibility with outside extensions, but it doesn't do anything within php-src. If you're writing a function that needs parameter parsing and which targets only PHP 7.0+, you now don't need to (and shouldn't) include zpp fallback code if you're using Fast ZPP. Thanks!
-- Andrea Faulds https://ajf.me/

Pierre Joye

9 years ago
Hi Andrea, On Sep 12, 2016 4:57 AM, "Andrea Faulds" <ajf@ajf.me> wrote:
> > Hi everyone, > > Andrea Faulds wrote: >> >> I finally wrote a patch to get rid of it: >> >> https://github.com/php/php-src/pull/2118 >> >> Perhaps this will go somewhere. :) > > > This has now been merged into PHP-7.0 and up: > >
https://github.com/php/php-src/commit/d690014bf35507ccb7a1150a27504d2f87848842
> > Additionally, extra zpp fallback code only found in master has been
removed here:
> >
https://github.com/php/php-src/commit/3cc90901010f016923e9ca6a4ae15ba08bc18169
> > Now there is no fallback zpp code around, and Fast ZPP is always enabled. > > The FAST_ZPP macro itself remains for compatibility with outside
extensions, but it doesn't do anything within php-src.
> > If you're writing a function that needs parameter parsing and which
targets only PHP 7.0+, you now don't need to (and shouldn't) include zpp fallback code if you're using Fast ZPP.
> > Thanks!
Thanks you :) Please add this note to UPGRADING.INTERNALS as well : Cheers Pierre

Andrew Faulds

9 years ago
Hi Pierre, Pierre Joye wrote:
> > Thanks you :) > > Please add this note to UPGRADING.INTERNALS as well :
Well, I was wondering about that, but I don't really know where it should be noted. It's not a user-facing change, in fact nobody was disabling FAST_ZPP anyway, so it couldn't be one. Therefore, it shouldn't be noted in NEWS. And UPGRADING.INTERNALS is an upgrading guide used between major or minor releases. This is a patch change that affects not only master, but 7.1 and 7.0. So it isn't suitable for that, either. Plus, UPGRADING.INTERNALS has no existing mention of Fast ZPP. So, it doesn't seem to go anywhere, unfortunately. :/ Thanks.
-- Andrea Faulds https://ajf.me/

Pierre Joye

9 years ago
On Sep 15, 2016 7:10 AM, "Andrea Faulds" <ajf@ajf.me> wrote:
> > Hi Pierre, > > > Pierre Joye wrote: >> >> >> Thanks you :) >> >> Please add this note to UPGRADING.INTERNALS as well : > > > Well, I was wondering about that, but I don't really know where it should
be noted.
> > It's not a user-facing change, in fact nobody was disabling FAST_ZPP
anyway, so it couldn't be one. Therefore, it shouldn't be noted in NEWS.
> > And UPGRADING.INTERNALS is an upgrading guide used between major or minor
releases. This is a patch change that affects not only master, but 7.1 and 7.0. So it isn't suitable for that, either.
> > Plus, UPGRADING.INTERNALS has no existing mention of Fast ZPP. > > So, it doesn't seem to go anywhere, unfortunately. :/ >
It does fit there. It was something we forgot to mention in 7.0 so let fix that now. Cheers Pierre

Christoph Becker

9 years ago
On 04.05.2016 at 18:11, Andrea Faulds wrote:
> Since FAST_ZPP was implemented, many functions in built-in PHP > extensions look like this: > > […] > > That is, they have /two/ sets of parameter processing code: one using > the old API, and one using the new API, with conditional compilation. > This is necessary because it is still possible to turn off FAST_ZPP by > changing the value of the macro. > > However, should that be the case? The FAST_ZPP RFC was approved and PHP > 7.0 was released containing it enabled by default, so it's no longer > just an experimental performance enhancement.
It seems to me that the FAST_ZPP macro is not (only) to disable an experimental feature, but rather to be able to reduce the size of the built binaries. I've just checked a `./configure --disable-all`: with FAST_ZPP: 16,651,912 without FAST_ZPP: 16,276,152 That's a difference of 375,760 bytes, but only 2% increase.
> Furthermore, I have no > reason to believe that the option to turn off FAST_ZPP is actually used > in practice, though feel free to prove me wrong!
Perhaps that is so, because FAST_ZPP can't be turned of via ./configure.
> Requiring all usages of FAST_ZPP to be accompanied by a > zend_parse_parameters() fallback creates code bloat. It also creates > hidden, potentially build-breaking bugs, because the code is rarely > built with FAST_ZPP disabled. It's like the old TSRMLS_CC situation. > > Furthermore, for those who would prefer the FAST_ZPP API, it is > inconvenient that they cannot use it exclusively and must also use > zend_parse_parameters(), doubling the effort.
Valid points.
> So, I would like to ask: would there be any objections to going through > and removing all unnecessary zend_parse_parameters fallbacks, and making > it impossible to disable FAST_ZPP? This would make the two APIs equally > valid.
All in all, I think that the benefits of using only one of the parameter parsing APIs per function outweigh the drawbacks, so I'm +1 on the removal of the FAST_ZPP macro.
-- Christoph M. Becker

Andrew Faulds

9 years ago
Hi Christoph, Christoph M. Becker wrote:
> On 04.05.2016 at 18:11, Andrea Faulds wrote: > > It seems to me that the FAST_ZPP macro is not (only) to disable an > experimental feature, but rather to be able to reduce the size of the > built binaries. I've just checked a `./configure --disable-all`: > > with FAST_ZPP: 16,651,912 > without FAST_ZPP: 16,276,152 > > That's a difference of 375,760 bytes, but only 2% increase.
This is a fair point. Functions using Fast ZPP have larger code size, because of aggressive inlining. If every function used it, I suppose it might bloat the size of the PHP binary somewhat.
> >> Furthermore, I have no >> reason to believe that the option to turn off FAST_ZPP is actually used >> in practice, though feel free to prove me wrong! > > Perhaps that is so, because FAST_ZPP can't be turned of via ./configure.
That's true, but I don't know if there was any reason to add such an option. In any case, thanks to some uncaught bugs in dead code, no stable release of PHP 7 has been able to compile with FAST_ZPP disabled anyway, which means I can be certain nobody is using it. Thanks for your input.
-- Andrea Faulds https://ajf.me/

Christoph Becker

9 years ago
Hi Andrea! On 07.09.2016 at 17:26, Andrea Faulds wrote:
> Christoph M. Becker wrote: > >> On 04.05.2016 at 18:11, Andrea Faulds wrote: > > This is a fair point. Functions using Fast ZPP have larger code size, > because of aggressive inlining. > > If every function used it, I suppose it might bloat the size of the PHP > binary somewhat.
And probably for no good reason. The performance overhead of zend_parse_parameters() can probably be ignored for a lot of functions, and while it is somewhat harder to read, besides it's being space-saving, it is way easier to debug. There are, however, functions that would greatly benefit from using the fast ZPP API (e.g. imagecolorat(), see commit 0bf11d1e), so I'd welcome code reviews regarding (fast) ZPP.
> In any case, thanks to some uncaught bugs in dead code, no stable > release of PHP 7 has been able to compile with FAST_ZPP disabled anyway, > which means I can be certain nobody is using it.
I have successfully compiled a recent master with FAST_ZPP undefined. While that was with `--disable-all`, I can imagine that some extensions would also compile, so perhaps somebody actually uses that feature. However, even if that is the case, I don't see necessity to do so.
-- Christoph M. Becker

Andrew Faulds

9 years ago
Hi Cristoph, Christoph M. Becker wrote:
>> In any case, thanks to some uncaught bugs in dead code, no stable >> release of PHP 7 has been able to compile with FAST_ZPP disabled anyway, >> which means I can be certain nobody is using it. > > I have successfully compiled a recent master with FAST_ZPP undefined. > While that was with `--disable-all`, I can imagine that some extensions > would also compile, so perhaps somebody actually uses that feature. > However, even if that is the case, I don't see necessity to do so.
Ah, yes, you can compile it now, but you couldn't a few days ago when the broken code that'd been in there since September hadn't yet been fixed. ;)
-- Andrea Faulds https://ajf.me/

Christoph Becker

9 years ago
Hi Andrea! On 10.09.2016 at 01:33, Andrea Faulds wrote:
> Christoph M. Becker wrote: > >>> In any case, thanks to some uncaught bugs in dead code, no stable >>> release of PHP 7 has been able to compile with FAST_ZPP disabled anyway, >>> which means I can be certain nobody is using it. >> >> I have successfully compiled a recent master with FAST_ZPP undefined. >> While that was with `--disable-all`, I can imagine that some extensions >> would also compile, so perhaps somebody actually uses that feature. >> However, even if that is the case, I don't see necessity to do so. > > Ah, yes, you can compile it now, but you couldn't a few days ago when > the broken code that'd been in there since September hadn't yet been > fixed. ;)
Oh, I see.
-- Christoph M. Becker