Add a rule to root-namespace special compiled PHP functions

[nicolas-grekas]

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

(don't miss #3048 (comment))

[bendavies]

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

[bendavies]

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]

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

[SpacePossum]

"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]

@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]

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

[keradus]

@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]

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]

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]

Cool, thanks for doing the hard work on this.

[keradus]

@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]

@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]

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]

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]

• wondering: https://twitter.com/Ocramius/status/811504929357660160
• indicator that it might make a difference: http://php.net/manual/en/language.namespaces.faq.php#language.namespaces.faq.shortname2 (basically what I said for 2-steps lookup)
• some benchmark: http://veewee.github.io/blog/optimizing-php-performance-by-fq-function-calls/
• some other benchmark even with \-adder https://github.com/Roave/FunctionFQNReplacer
• opcode difference between non-\ vs \ is INIT_NS_FCALL_BY_NAME vs INIT_FCALL and so (totally different opcodes of resolving function, passing arguments and calling functoin)

[SpacePossum]

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]

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]

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]

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

[nicolas-grekas]

@keradus ??????

[keradus]

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]

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]

or add an include option, WIP SpacePossum@2490fb1