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 a rule to root-namespace special compiled PHP functions #3048

Closed
nicolas-grekas opened this Issue Sep 12, 2017 · 40 comments

Comments

Projects
None yet
@nicolas-grekas

nicolas-grekas commented Sep 12, 2017

Since PHP 7.0, some functions are replaced by opcodes, producing much faster code.
Yet, for this to work, these functions need to be referenced in the root namespace at compile time:
Either there is no namespace, or they are prefixed by a \.
php-cs-fixer should have a rule to list those functions and enforce adding this \ when used inside a namespaced file/class.

Here is the exact list of functions that have this behavior:
https://github.com/php/php-src/blob/f2db305fa4e9bd7d04d567822687ec714aedcdb5/Zend/zend_compile.c#L3872

@bendavies

This comment has been minimized.

Show comment
Hide comment
@bendavies

bendavies Sep 12, 2017

Contributor

I was just about to raise this. you beat me to it :)

Contributor

bendavies commented Sep 12, 2017

I was just about to raise this. you beat me to it :)

@bendavies

This comment has been minimized.

Show comment
Hide comment
@bendavies

bendavies Sep 12, 2017

Contributor

btw, this is already covered by the native_function_invocation rule:
https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/master/src/Fixer/FunctionNotation/NativeFunctionInvocationFixer.php

but as you said, it could have extra configuration to only replace the function calls you link to.

Contributor

bendavies commented Sep 12, 2017

btw, this is already covered by the native_function_invocation rule:
https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/master/src/Fixer/FunctionNotation/NativeFunctionInvocationFixer.php

but as you said, it could have extra configuration to only replace the function calls you link to.

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Sep 12, 2017

Member

looks like a nice scenario
please open a PR to improve configuration of the fixer @nicolas-grekas 👍

Member

keradus commented Sep 12, 2017

looks like a nice scenario
please open a PR to improve configuration of the fixer @nicolas-grekas 👍

@SpacePossum

This comment has been minimized.

Show comment
Hide comment
@SpacePossum

SpacePossum Sep 12, 2017

Member
"array_slice"
"assert"
"boolval"
"call_user_func"
"call_user_func_array"
"chr"
"count"
"defined"
"doubleval"
"floatval"
"func_get_args"
"func_num_args"
"get_called_class"
"get_class"
"gettype"
"in_array"
"intval"
"is_array"
"is_bool"
"is_double"
"is_float"
"is_int"
"is_integer"
"is_long"
"is_null"
"is_object"
"is_real"
"is_resource"
"is_string"
"ord"
"strlen"
"strval"

IIUC

Member

SpacePossum commented Sep 12, 2017

"array_slice"
"assert"
"boolval"
"call_user_func"
"call_user_func_array"
"chr"
"count"
"defined"
"doubleval"
"floatval"
"func_get_args"
"func_num_args"
"get_called_class"
"get_class"
"gettype"
"in_array"
"intval"
"is_array"
"is_bool"
"is_double"
"is_float"
"is_int"
"is_integer"
"is_long"
"is_null"
"is_object"
"is_real"
"is_resource"
"is_string"
"ord"
"strlen"
"strval"

IIUC

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 12, 2017

@SpacePossum correct, with some subtleties if we want to deal with them (just discovered that eg in_array is optimized only if its 3rd arg is set)

nicolas-grekas commented Sep 12, 2017

@SpacePossum correct, with some subtleties if we want to deal with them (just discovered that eg in_array is optimized only if its 3rd arg is set)

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Sep 12, 2017

Member

haha, next scenario when it's always better to use strict comparisons ;)

Member

keradus commented Sep 12, 2017

haha, next scenario when it's always better to use strict comparisons ;)

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Sep 17, 2017

Member

@nicolas-grekas

native_function_invocation adds \ to each native function invocation.
this makes them to be handled faster, as some of them will be converted to opcodes, and the rest will be resolved immediately as root function instead of having a need to check if the function is also registered in current namespace.

is there anything left for this fixer ?

Member

keradus commented Sep 17, 2017

@nicolas-grekas

native_function_invocation adds \ to each native function invocation.
this makes them to be handled faster, as some of them will be converted to opcodes, and the rest will be resolved immediately as root function instead of having a need to check if the function is also registered in current namespace.

is there anything left for this fixer ?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 17, 2017

For the purpose of Symfony, I may advocate that a rule that enforces the backslash when it provides something substential should be enabled.
But I will never advocate that all functions should be prefixed.
Is that what the linked PR provides? Would it be possible to select/configure the list of root functions that should be prefixed? If yes, then we might have a deal :)

nicolas-grekas commented Sep 17, 2017

For the purpose of Symfony, I may advocate that a rule that enforces the backslash when it provides something substential should be enabled.
But I will never advocate that all functions should be prefixed.
Is that what the linked PR provides? Would it be possible to select/configure the list of root functions that should be prefixed? If yes, then we might have a deal :)

@SpacePossum

This comment has been minimized.

Show comment
Hide comment
@SpacePossum

SpacePossum Sep 17, 2017

Member

I have a branch that will provide what you are after, however it is built upon the one linked PR, the linked PR itself does not.

Member

SpacePossum commented Sep 17, 2017

I have a branch that will provide what you are after, however it is built upon the one linked PR, the linked PR itself does not.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 17, 2017

Cool, thanks for doing the hard work on this.

nicolas-grekas commented Sep 17, 2017

Cool, thanks for doing the hard work on this.

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Sep 17, 2017

Member

@nicolas-grekas , what about later part?

and the rest will be resolved immediately as root function instead of having a need to check if the function is also registered in current namespace

Member

keradus commented Sep 17, 2017

@nicolas-grekas , what about later part?

and the rest will be resolved immediately as root function instead of having a need to check if the function is also registered in current namespace

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 17, 2017

@keradus not sure what you mean? If you mean devs should be required to always add the backslash prefix, I certainly don't agree, there is zero perf benefit doing so, but only more visual debt in the code...

nicolas-grekas commented Sep 17, 2017

@keradus not sure what you mean? If you mean devs should be required to always add the backslash prefix, I certainly don't agree, there is zero perf benefit doing so, but only more visual debt in the code...

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Sep 17, 2017

Member

I'm saying that there is benefit, as if you add \ to all native functions, you will get benefit of php not needed to check if you have function in given namespace, as you explicitly say you don't care about current namespace, but you care about root-level native function. so there is benefit even if function call cannot be converted to opcode

Member

keradus commented Sep 17, 2017

I'm saying that there is benefit, as if you add \ to all native functions, you will get benefit of php not needed to check if you have function in given namespace, as you explicitly say you don't care about current namespace, but you care about root-level native function. so there is benefit even if function call cannot be converted to opcode

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 17, 2017

Did you try measuring it actually? Last time I did, I couldn't spot any difference at all, except for the specific functions listed above.

nicolas-grekas commented Sep 17, 2017

Did you try measuring it actually? Last time I did, I couldn't spot any difference at all, except for the specific functions listed above.

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Sep 17, 2017

Member
Member

keradus commented Sep 17, 2017

@SpacePossum

This comment has been minimized.

Show comment
Hide comment
@SpacePossum

SpacePossum Sep 18, 2017

Member

If it does make a difference for all calls:

  • why doesn't it show in the op codes?
  • why is this fixer inconsistent, i.e. only fix native function calls? https://3v4l.org/Z0NuO/vld#output shows calling user defined functions in global namespace also benefit from the \

Why can't this be a different issue report so the same discussion is not done two times, here and in the PR? This issue is about a special subset of native calls, the PR about scoping the namespacing, I don't see the need to widen the scope.

Member

SpacePossum commented Sep 18, 2017

If it does make a difference for all calls:

  • why doesn't it show in the op codes?
  • why is this fixer inconsistent, i.e. only fix native function calls? https://3v4l.org/Z0NuO/vld#output shows calling user defined functions in global namespace also benefit from the \

Why can't this be a different issue report so the same discussion is not done two times, here and in the PR? This issue is about a special subset of native calls, the PR about scoping the namespacing, I don't see the need to widen the scope.

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Sep 18, 2017

Member

why doesn't it show in the op codes?

it does, like INIT_NS_FCALL_BY_NAME vs INIT_FCALL
amount of opcodes are the same, but the opcodes itself are different

why is this fixer inconsistent, i.e. only fix native function calls?

As in scope of file, we are unaware of all user-defined functions :( we know for sure that native functions are defined in root scope and hope they are not overwritten in namespace (which could be in different file). We cannot be sure that user-defined function is declared in root scope.

Why can't this be a different issue report so the same discussion is not done two times (...)

For sure it's worth to not work on it twice, yet before implementing I want to be sure this part it's needed (configuring to fix only subset). Thus I took discussion-first approach.

Member

keradus commented Sep 18, 2017

why doesn't it show in the op codes?

it does, like INIT_NS_FCALL_BY_NAME vs INIT_FCALL
amount of opcodes are the same, but the opcodes itself are different

why is this fixer inconsistent, i.e. only fix native function calls?

As in scope of file, we are unaware of all user-defined functions :( we know for sure that native functions are defined in root scope and hope they are not overwritten in namespace (which could be in different file). We cannot be sure that user-defined function is declared in root scope.

Why can't this be a different issue report so the same discussion is not done two times (...)

For sure it's worth to not work on it twice, yet before implementing I want to be sure this part it's needed (configuring to fix only subset). Thus I took discussion-first approach.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 18, 2017

On the very first call, opcache resolves the namespace once for all and replaces the slower opcode by the fully qualified one. This hot replacement is free compared to running whatever the rest of the code does.
The benchmark you linked doesn't say anything about opcache being enabled or not. The php --version doesn't show it, and CLI disables it by default, so it's likely it was not enabled.
In short, I don't buy it at all. And the other links prove absolutely nothing.

nicolas-grekas commented Sep 18, 2017

On the very first call, opcache resolves the namespace once for all and replaces the slower opcode by the fully qualified one. This hot replacement is free compared to running whatever the rest of the code does.
The benchmark you linked doesn't say anything about opcache being enabled or not. The php --version doesn't show it, and CLI disables it by default, so it's likely it was not enabled.
In short, I don't buy it at all. And the other links prove absolutely nothing.

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Sep 18, 2017

Member

you see, it's not hard to discuss ;)
in that case, feel free to open a PR @nicolas-grekas

Member

keradus commented Sep 18, 2017

you see, it's not hard to discuss ;)
in that case, feel free to open a PR @nicolas-grekas

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas commented Sep 18, 2017

@keradus ??????

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Sep 18, 2017

Member

what's up?
here, I challenged current fixer we do have and reasoning we had behind him.
with your last reasoning, I think we could change default behaviour of it to fix only opcache-friendly functions by default (3.x line)

Member

keradus commented Sep 18, 2017

what's up?
here, I challenged current fixer we do have and reasoning we had behind him.
with your last reasoning, I think we could change default behaviour of it to fix only opcache-friendly functions by default (3.x line)

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Sep 25, 2017

There is the exclude option already, so why not add another configuration option in the 2.x line?

[
    'native_function_invocation' => [
        'exclude' => [],
        'opcache-only' => true,
    ]
]

That would be forward compatible with version 3.

leofeyer commented Sep 25, 2017

There is the exclude option already, so why not add another configuration option in the 2.x line?

[
    'native_function_invocation' => [
        'exclude' => [],
        'opcache-only' => true,
    ]
]

That would be forward compatible with version 3.

@SpacePossum

This comment has been minimized.

Show comment
Hide comment
@SpacePossum

SpacePossum Sep 25, 2017

Member

or add an include option, WIP SpacePossum@2490fb1

Member

SpacePossum commented Sep 25, 2017

or add an include option, WIP SpacePossum@2490fb1

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 28, 2017

Here is new stuff I just learned about from @jpauli: OPcache does additional inlining, see
https://github.com/php/php-src/blob/master/ext/opcache/Optimizer/pass1_5.c

extra list is:

  • function_exists
  • is_callable
  • extension_loaded
  • dirname
  • constant
  • define

nicolas-grekas commented Sep 28, 2017

Here is new stuff I just learned about from @jpauli: OPcache does additional inlining, see
https://github.com/php/php-src/blob/master/ext/opcache/Optimizer/pass1_5.c

extra list is:

  • function_exists
  • is_callable
  • extension_loaded
  • dirname
  • constant
  • define
@SpacePossum

This comment has been minimized.

Show comment
Hide comment
@SpacePossum

SpacePossum Sep 28, 2017

Member

Thanks, I'll update the functions list in my branch :)

Btw. my POC PR @ SF did already only change the subset of functions as listed here and only within a user-defined-namespace.

Member

SpacePossum commented Sep 28, 2017

Thanks, I'll update the functions list in my branch :)

Btw. my POC PR @ SF did already only change the subset of functions as listed here and only within a user-defined-namespace.

@theofidry

This comment has been minimized.

Show comment
Hide comment
@theofidry

theofidry Jan 22, 2018

Maybe instead of escaping native functions what about adding them with a use statement?

use function is_array;

is_array($x);

// Instead of:

\is_array($x);

It feels way more natural and make it easier to read IMO. Also for reference this is already used in doctrine/coding-standard#15 and you also can enable it via your IDE (cf the doctrine coding standard issue)

theofidry commented Jan 22, 2018

Maybe instead of escaping native functions what about adding them with a use statement?

use function is_array;

is_array($x);

// Instead of:

\is_array($x);

It feels way more natural and make it easier to read IMO. Also for reference this is already used in doctrine/coding-standard#15 and you also can enable it via your IDE (cf the doctrine coding standard issue)

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Jan 22, 2018

Contributor

@theofidry this cannot be done in the LTS branch of Symfony, as it makes the code incompatible with PHP 5.x. Using the fully-qualified function name works on PHP 5.3+ (it won't have the same optimizations on older versions, but that's not an issue). So Symfony is more interested in qualifying the call (making this configurable is an option though)

Contributor

stof commented Jan 22, 2018

@theofidry this cannot be done in the LTS branch of Symfony, as it makes the code incompatible with PHP 5.x. Using the fully-qualified function name works on PHP 5.3+ (it won't have the same optimizations on older versions, but that's not an issue). So Symfony is more interested in qualifying the call (making this configurable is an option though)

@theofidry

This comment has been minimized.

Show comment
Hide comment
@theofidry

theofidry Jan 22, 2018

Sure, but not everyone is stuck with that 5.x compatibility issue. I simply worry that everyone will jump on that option when available making a lot of code unreadable for no reason; Ideally a choice between the two options would be given so that depending of your needs you can pick the right solution.

Also I would argue you should use use, use const & use function for anything, not just to optimise some native calls

theofidry commented Jan 22, 2018

Sure, but not everyone is stuck with that 5.x compatibility issue. I simply worry that everyone will jump on that option when available making a lot of code unreadable for no reason; Ideally a choice between the two options would be given so that depending of your needs you can pick the right solution.

Also I would argue you should use use, use const & use function for anything, not just to optimise some native calls

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Jan 22, 2018

Contributor

@theofidry the SF coding standards are also to avoid the class use statement for the global namespace btw, so using \ for function would even be consistent. So this definitely need to be cpnfigurable IMO.

Contributor

stof commented Jan 22, 2018

@theofidry the SF coding standards are also to avoid the class use statement for the global namespace btw, so using \ for function would even be consistent. So this definitely need to be cpnfigurable IMO.

@gggeek

This comment has been minimized.

Show comment
Hide comment
@gggeek

gggeek Jan 24, 2018

Maybe it's just the mental burden of having to abandon long time habits, but the idea of having to prefix usage of every "standard" function call (or even just a small but not negligible part of them) makes me shudder. The solution proposed by @theofidry seems much cleaner. Or maybe just wait for the php engine to implement a pragma that disables dynamic function name resolution for the current file and have this fixed with a one-liner change...

gggeek commented Jan 24, 2018

Maybe it's just the mental burden of having to abandon long time habits, but the idea of having to prefix usage of every "standard" function call (or even just a small but not negligible part of them) makes me shudder. The solution proposed by @theofidry seems much cleaner. Or maybe just wait for the php engine to implement a pragma that disables dynamic function name resolution for the current file and have this fixed with a one-liner change...

@SpacePossum

This comment has been minimized.

Show comment
Hide comment
@SpacePossum

SpacePossum Jan 24, 2018

Member

We can offer both options and than people/communities can decide what style they want to us,
this typically works best with high opinionated CS preferences (like Yoda-style as well ;)
It would move the discussion about the CS to the communities and away from here where the exposure might be less visible.

Member

SpacePossum commented Jan 24, 2018

We can offer both options and than people/communities can decide what style they want to us,
this typically works best with high opinionated CS preferences (like Yoda-style as well ;)
It would move the discussion about the CS to the communities and away from here where the exposure might be less visible.

@derrabus

This comment has been minimized.

Show comment
Hide comment
@derrabus

derrabus Jan 24, 2018

Or maybe just wait for the php engine to implement a pragma that disables dynamic function name resolution for the current file

We don't know if that will ever happen. And even if, it would take years until that feature will be usable for frameworks and libraries. That does not sound like a good plan.

derrabus commented Jan 24, 2018

Or maybe just wait for the php engine to implement a pragma that disables dynamic function name resolution for the current file

We don't know if that will ever happen. And even if, it would take years until that feature will be usable for frameworks and libraries. That does not sound like a good plan.

@gggeek

This comment has been minimized.

Show comment
Hide comment
@gggeek

gggeek Jan 24, 2018

"We don't know if that will ever happen" => sure, that was more of a tongue-in-cheek joke than a real proposal ;-)

gggeek commented Jan 24, 2018

"We don't know if that will ever happen" => sure, that was more of a tongue-in-cheek joke than a real proposal ;-)

@glensc

This comment has been minimized.

Show comment
Hide comment
@glensc

glensc May 2, 2018

Contributor

@nicolas-grekas pointed out to this issue, that not all functions are slashed by the fixer.

symfony/polyfill-mbstring@3296adf

  • \floor
  • \gettype
  • \strpos
  • \iconv
  • \trigger_error
Contributor

glensc commented May 2, 2018

@nicolas-grekas pointed out to this issue, that not all functions are slashed by the fixer.

symfony/polyfill-mbstring@3296adf

  • \floor
  • \gettype
  • \strpos
  • \iconv
  • \trigger_error
@ntzm

This comment has been minimized.

Show comment
Hide comment
@ntzm

ntzm May 2, 2018

Contributor

Are they compiler optimised functions?

Contributor

ntzm commented May 2, 2018

Are they compiler optimised functions?

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof May 2, 2018

Contributor

@glensc this feature request is precisely about not prefixing them all, but prefixing only the compiler-optimized ones. Prefixing them all is already possible (but is not what the Symfony team wants to do).

Contributor

stof commented May 2, 2018

@glensc this feature request is precisely about not prefixing them all, but prefixing only the compiler-optimized ones. Prefixing them all is already possible (but is not what the Symfony team wants to do).

keradus added a commit that referenced this issue 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

@gharlan

This comment has been minimized.

Show comment
Hide comment
@gharlan

gharlan Jun 5, 2018

Contributor

Maybe instead of escaping native functions what about adding them with a use statement?

use function is_array;

is_array($x);

// Instead of:

\is_array($x);

I would like to use this style. Should it be an option in this fixer? Or maybe better another fixer, something like import_root_namespaced_functions?

Contributor

gharlan commented Jun 5, 2018

Maybe instead of escaping native functions what about adding them with a use statement?

use function is_array;

is_array($x);

// Instead of:

\is_array($x);

I would like to use this style. Should it be an option in this fixer? Or maybe better another fixer, something like import_root_namespaced_functions?

@jaydiablo

This comment has been minimized.

Show comment
Hide comment
@jaydiablo

jaydiablo Jun 5, 2018

Contributor

@gharlan There's some discussion about that in this issue: #2739

Seems like it needs to be decided if it's an option to the native_function_invocation fixer, or a separate fixer, as you suggested.

Contributor

jaydiablo commented Jun 5, 2018

@gharlan There's some discussion about that in this issue: #2739

Seems like it needs to be decided if it's an option to the native_function_invocation fixer, or a separate fixer, as you suggested.

@gharlan

This comment has been minimized.

Show comment
Hide comment
@gharlan

gharlan Jun 5, 2018

Contributor

@gharlan There's some discussion about that in this issue: #2739

Ah thanks! 👍

Contributor

gharlan commented Jun 5, 2018

@gharlan There's some discussion about that in this issue: #2739

Ah thanks! 👍

REBELinBLUE referenced this issue in REBELinBLUE/deployer Jun 27, 2018

REBELinBLUE referenced this issue in REBELinBLUE/deployer Jun 27, 2018

REBELinBLUE referenced this issue in REBELinBLUE/deployer Jun 27, 2018

REBELinBLUE referenced this issue in REBELinBLUE/deployer Jun 27, 2018

REBELinBLUE referenced this issue in REBELinBLUE/deployer Jun 27, 2018

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