Re: The death of `#ifndef FAST_ZPP`?

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