opcache: re-enable PASS_15 (constant collection) by default#22007
Open
wilaak wants to merge 2 commits into
Open
opcache: re-enable PASS_15 (constant collection) by default#22007wilaak wants to merge 2 commits into
wilaak wants to merge 2 commits into
Conversation
wilaak
force-pushed
the
fix/inline-file-scope-const-declarations-master
branch
from
e57f1f4 to
c5e2cf5
Compare
Disabled in Dec 2015 (940c68b, 4070279) to fix bug #71127. define() and attributed constants are blocked from inlining via a sentinel entry. define() can be overridden at runtime (bug #71127) and attributed constants need to fire at access time. The sentinel also prevents inlining of any subsequent const redeclaration of the same name. Adds zend_optimizer_block_constant() for sentinel logic.
wilaak
force-pushed
the
fix/inline-file-scope-const-declarations-master
branch
from
c5e2cf5 to
35cd64d
Compare
| --FILE-- | ||
| <?php | ||
|
|
||
| const CONST_VAL = 1; |
Member
There was a problem hiding this comment.
please also add tests for what happens if the constant is already declared in a separate file
Author
There was a problem hiding this comment.
If the constant is only declared in the separate file the optimizer can't inline it since the value is unknown at compile time, so it emits FETCH_CONSTANT.
If you also redeclare it locally the optimizer inlines the local value, but at runtime the required declaration wins and the local one warns. You get different results with and without opcache, so there's a real divergence there!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PASS_15 was disabled in Dec 2015 (940c68b, 4070279) to fix bug #71127.
define() values are not inlined; a sentinel entry also blocks inlining of any subsequent const redeclaration of the same name. Attributed constants are similarly blocked since their attributes need to fire at access time.
Not 100% sure every edge case is covered or if this is a good approach.
Dmitry, does this look safe to you?
Probably not worth it for the tiny benefit it provides. It only provides perf gains if you were to define and use it in the same file. Should I close this as it's a better idea to defer until the optimizer can do whole-program analysis?