Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » Remove useless opcode handlers »

Remove useless opcode handlers

php.internals

Dmitry Stogov

10 years ago
Please take a quick look and let me know if you see any problems. https://gist.github.com/dstogov/23cc318dd3e411904e10 I'm going to commit this tomorrow. Thanks. Dmitry.

Nikita Popov

10 years ago
On Mon, Mar 14, 2016 at 11:56 AM, Dmitry Stogov <dmitry@zend.com> wrote:
> Please take a quick look and let me know if you see any problems. > > > https://gist.github.com/dstogov/23cc318dd3e411904e10 > > > I'm going to commit this tomorrow. > > > Thanks. Dmitry. >
ZEND_ADD is not commutative for array operands. ZEND_MUL may not be commutative if operator overloading is involved (e.g. matrix multiplication is not abelian). The NO_CONST_CONST cases are problematic: Opcache currently, in my eyes incorrectly, evaluates constant expressions even if they throw notices (currently I think mostly by suppressing them). If we do not evaluate these, we may end up with CONST_CONST operations. (This is not a huge problem just now because constant folding in opcache is limited -- however with full constant propagation this leads to many test failures.) The opcodes affected by the last point (limited to those included in your patch) are SUB, MUL, POW, CONCAT (array operands) and BW_NOT (various). However with Andrea's invalid numeric string RFC more will fall in this category (evaluable but throwing). Nikita

Dmitry Stogov

10 years ago
On 03/14/2016 02:29 PM, Nikita Popov wrote:
> On Mon, Mar 14, 2016 at 11:56 AM, Dmitry Stogov <dmitry@zend.com > <mailto:dmitry@zend.com>> wrote: > > Please take a quick look and let me know if you see any problems. > > > https://gist.github.com/dstogov/23cc318dd3e411904e10 > > > I'm going to commit this tomorrow. > > > Thanks. Dmitry. > > ZEND_ADD is not commutative for array operands.
Unfortunately, you are right :(
> ZEND_MUL may not be commutative if operator overloading is involved > (e.g. matrix multiplication is not abelian).
Right again.
> The NO_CONST_CONST cases are problematic: Opcache currently, in my > eyes incorrectly, evaluates constant expressions even if they throw > notices (currently I think mostly by suppressing them). If we do not > evaluate these, we may end up with CONST_CONST operations. (This is > not a huge problem just now because constant folding in opcache is > limited -- however with full constant propagation this leads to many > test failures.)
The patch adds NO_CONST_CONST handling to opcache. All tests are passed, I also didn't see any problems running ~10 real-life apps.
> The opcodes affected by the last point (limited to those included in > your patch) are SUB, MUL, POW, CONCAT (array operands) and BW_NOT > (various). However with Andrea's invalid numeric string RFC more will > fall in this category (evaluable but throwing).
I didn't get what is wrong with SUB, MUL, etc, because they are evaluated at compile-time Anyway, I see - the patch can't be committed in current state. Thanks for review. Dmitry.

Nikita Popov

10 years ago
On Mon, Mar 14, 2016 at 12:58 PM, Dmitry Stogov <dmitry@zend.com> wrote:
> > > On 03/14/2016 02:29 PM, Nikita Popov wrote: > > On Mon, Mar 14, 2016 at 11:56 AM, Dmitry Stogov <dmitry@zend.com> wrote: > >> Please take a quick look and let me know if you see any problems. >> >> >> https://gist.github.com/dstogov/23cc318dd3e411904e10 >> >> >> I'm going to commit this tomorrow. >> >> >> Thanks. Dmitry. >> > ZEND_ADD is not commutative for array operands. > > > Unfortunately, you are right :( > > ZEND_MUL may not be commutative if operator overloading is involved (e.g. > matrix multiplication is not abelian). > > > Right again. > > The NO_CONST_CONST cases are problematic: Opcache currently, in my eyes > incorrectly, evaluates constant expressions even if they throw notices > (currently I think mostly by suppressing them). If we do not evaluate > these, we may end up with CONST_CONST operations. (This is not a huge > problem just now because constant folding in opcache is limited -- however > with full constant propagation this leads to many test failures.) > > > The patch adds NO_CONST_CONST handling to opcache. All tests are passed, I > also didn't see any problems running ~10 real-life apps. > > The opcodes affected by the last point (limited to those included in your > patch) are SUB, MUL, POW, CONCAT (array operands) and BW_NOT (various). > However with Andrea's invalid numeric string RFC more will fall in this > category (evaluable but throwing). > > > I didn't get what is wrong with SUB, MUL, etc, because they are evaluated > at compile-time > Anyway, I see - the patch can't be committed in current state. > Thanks for review. >
What I mean is, for example: var_dump(~(bool)true); echo "Done\n"; Will throw an exception without opcache, will result in no output (and no *visible* error) with opcache. My point here is that const-operand-only cases might be necessary to handle error cases like this. Nikita

Andrew Faulds

10 years ago
Hi, Nikita Popov wrote:
> What I mean is, for example: > > var_dump(~(bool)true); > echo "Done\n"; > > Will throw an exception without opcache, will result in no output (and no > *visible* error) with opcache. My point here is that const-operand-only > cases might be necessary to handle error cases like this.
It hasn't passed yet, but I think it's worth bringing up this RFC of mine here: https://wiki.php.net/rfc/invalid_strings_in_arithmetic It would require CONST-CONST to be valid for all arithmetic operations, in order to trigger a E_NOTICE/E_WARNING at runtime for certain (admittedly unlikely) cases. Thanks.
-- Andrea Faulds https://ajf.me/