New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the "opcache-only" option to the native function invocation fixer #3222

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@leofeyer

leofeyer commented Nov 7, 2017

This PR fixes #3048 by adding an "opcache-only" option to the native function invocation fixer. The option is set to false by default to maintain backwards compatibility.

@SpacePossum

This comment has been minimized.

Show comment
Hide comment
@SpacePossum

SpacePossum Nov 7, 2017

Member

similar as prep'ed (but was not PR'ed yet) #3223

Member

SpacePossum commented Nov 7, 2017

similar as prep'ed (but was not PR'ed yet) #3223

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 8, 2017

Would it be possible (and easy) to add a config option where we could list some constants that should be also prefixed? There are a few that would benefit from compile time resolution (eg DIRECTORY_SEPARATOR or PHP_VERSION_ID, which are the two that we use quite often and that OPcache could optimize by removing dead code)

nicolas-grekas commented Nov 8, 2017

Would it be possible (and easy) to add a config option where we could list some constants that should be also prefixed? There are a few that would benefit from compile time resolution (eg DIRECTORY_SEPARATOR or PHP_VERSION_ID, which are the two that we use quite often and that OPcache could optimize by removing dead code)

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Nov 8, 2017

I think this is already been dealt with in #3127. Maybe @Slamdunk can add an opcache-only option as well?

Actually, the native constants fixer is new, so we should not add the option there but only prefix constants that can be optimized. The whole purpose of the opcache-only option is to remain backwards compatible, so we don't have to wait for PHP-CS-Fixer version 3 to correctly optimize native function calls.

leofeyer commented Nov 8, 2017

I think this is already been dealt with in #3127. Maybe @Slamdunk can add an opcache-only option as well?

Actually, the native constants fixer is new, so we should not add the option there but only prefix constants that can be optimized. The whole purpose of the opcache-only option is to remain backwards compatible, so we don't have to wait for PHP-CS-Fixer version 3 to correctly optimize native function calls.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 8, 2017

#3127 looks good, I forgot about it :) no need for opcache-only there, as I'm not sure this would be well-defined at all :)

nicolas-grekas commented Nov 8, 2017

#3127 looks good, I forgot about it :) no need for opcache-only there, as I'm not sure this would be well-defined at all :)

nicolas-grekas added a commit to symfony/symfony that referenced this pull request Nov 8, 2017

minor #24866 Micro optim using explicit root namespaces (nicolas-grekas)
This PR was merged into the 3.4 branch.

Discussion
----------

Micro optim using explicit root namespaces

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Just doing that makes my local hello world as fast on 3.3 as on 4.0.
Spotted using Blackfire to identify the hot path.
Confirmed using both `ab` and `blackfire curl` on a local `php -S`.

It's not the first time these root namespaces make a measurable difference (on a selected list of functions only, see FriendsOfPHP/PHP-CS-Fixer#3048.)

FriendsOfPHP/PHP-CS-Fixer#3222 might become a more generic fix for this kind of optims.

Commits
-------

e78d1c4 Micro optim using explicit root namespaces
@Slamdunk

This comment has been minimized.

Show comment
Hide comment
@Slamdunk

Slamdunk Nov 8, 2017

Contributor

@nicolas-grekas I am open to make #3127 configurable, setting opcache-only by default. I only need someone to pinpoint me where to look at php-src for those special constants list.

Contributor

Slamdunk commented Nov 8, 2017

@nicolas-grekas I am open to make #3127 configurable, setting opcache-only by default. I only need someone to pinpoint me where to look at php-src for those special constants list.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 8, 2017

those special constants list

what I meant before: none are special (from OPcache POV)

nicolas-grekas commented Nov 8, 2017

those special constants list

what I meant before: none are special (from OPcache POV)

leofeyer added some commits Nov 14, 2017

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Dec 6, 2017

The failing Circle-CI tests are not related to this PR:

PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 69632 bytes) in /Users/distiller/PHP-CS-Fixer/src/Fixer/FunctionNotation/NativeFunctionInvocationFixer.php on line 222

@keradus @SpacePossum Is there anything I can do to get this PR merged?

leofeyer commented Dec 6, 2017

The failing Circle-CI tests are not related to this PR:

PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 69632 bytes) in /Users/distiller/PHP-CS-Fixer/src/Fixer/FunctionNotation/NativeFunctionInvocationFixer.php on line 222

@keradus @SpacePossum Is there anything I can do to get this PR merged?

@SpacePossum SpacePossum added the feature label Dec 6, 2017

@SpacePossum

This comment has been minimized.

Show comment
Hide comment
@SpacePossum

SpacePossum Dec 6, 2017

Member

As #3223 competes here and because of previous discussions on this topic in general my opinion/review could be biased, therefore I hope the other members pick this one up. That being said, thanks for all the work man, much appreciated 👍

Member

SpacePossum commented Dec 6, 2017

As #3223 competes here and because of previous discussions on this topic in general my opinion/review could be biased, therefore I hope the other members pick this one up. That being said, thanks for all the work man, much appreciated 👍

@keradus keradus changed the base branch from 2.8 to master Dec 26, 2017

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Feb 7, 2018

Since this PR will only be merged in version 3, I have backported the optimization for PHP-CS-Fixer 2:

https://github.com/leofeyer/optimize-native-functions-fixer

leofeyer commented Feb 7, 2018

Since this PR will only be merged in version 3, I have backported the optimization for PHP-CS-Fixer 2:

https://github.com/leofeyer/optimize-native-functions-fixer

@julienfalque

This comment has been minimized.

Show comment
Hide comment
@julienfalque

julienfalque Feb 7, 2018

Member

Maybe I'm missing something but why would this PR be merged in 3.x only?

Member

julienfalque commented Feb 7, 2018

Maybe I'm missing something but why would this PR be merged in 3.x only?

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Feb 7, 2018

@keradus has commented here and he has targeted the PR against the master branch.

leofeyer commented Feb 7, 2018

@keradus has commented here and he has targeted the PR against the master branch.

@julienfalque

This comment has been minimized.

Show comment
Hide comment
@julienfalque

julienfalque Feb 7, 2018

Member

IIUC, @keradus said that the default behavior of the fixer could be changed in version 3, but nothing prevents from adding new features in fixers of 2.x versions, as long as they are opt-in and don't change the default behavior. That means using an option with a default value that matches the current behavior of the fixer, which seems to be the case here :)

Member

julienfalque commented Feb 7, 2018

IIUC, @keradus said that the default behavior of the fixer could be changed in version 3, but nothing prevents from adding new features in fixers of 2.x versions, as long as they are opt-in and don't change the default behavior. That means using an option with a default value that matches the current behavior of the fixer, which seems to be the case here :)

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Feb 7, 2018

@julienfalque That's how I look at it, too. 😄

leofeyer commented Feb 7, 2018

@julienfalque That's how I look at it, too. 😄

keradus added a commit that referenced this pull request May 31, 2018

feature #3223 NativeFunctionInvocationFixer - add namespace scope and…
… include sets (SpacePossum)

This PR was squashed before being merged into the 2.12-dev branch (closes #3223).

Discussion
----------

NativeFunctionInvocationFixer - add namespace scope and include sets

replaces / closes #3222 (this PR adds the scoping for namespaces as well)
closes #3048

Commits
-------

4bb308e NativeFunctionInvocationFixer - add namespace scope and include sets

@keradus keradus closed this May 31, 2018

@SpacePossum

This comment has been minimized.

Show comment
Hide comment
@SpacePossum

SpacePossum Jun 1, 2018

Member

This PR got auto closed because of the merge of another PR bringing similar functionality, so in the end this idea/concept and setup are much appreciated :)
Thanks for all work you put in this PR @leofeyer !

Member

SpacePossum commented Jun 1, 2018

This PR got auto closed because of the merge of another PR bringing similar functionality, so in the end this idea/concept and setup are much appreciated :)
Thanks for all work you put in this PR @leofeyer !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment