Skip to content
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

Enable OptimizedFunctionsWithoutUnpacking sniff #89

Merged
merged 1 commit into from
Sep 23, 2018

Conversation

Majkl578
Copy link
Contributor

From readme:

PHP optimizes some internal functions into special opcodes on VM level. Such optimization results in much faster execution compared to calling standard function. This only works when these functions are not invoked with argument unpacking (...).

The list of these functions varies across PHP versions, but is the same as functions that must be referenced by their global name (either by \ prefix or using use function), not a fallback name inside namespaced code.

@Majkl578 Majkl578 added this to the 5.0.0 milestone Sep 23, 2018
@Majkl578 Majkl578 requested a review from a team as a code owner September 23, 2018 05:42
@Majkl578 Majkl578 force-pushed the sniff/OptimizedFunctionsWithoutUnpacking branch from a49a59e to d817715 Compare September 23, 2018 05:48
Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Majkl578 this one looks quite interesting! How would it behave with something like:

$args = [10, 'testing'];

echo sprintf('blah blah %d blah: %s', ...$args);

@Majkl578
Copy link
Contributor Author

Any form of unpacking kills the optimization. sprintf() is not on the list so it's not affected. Your example is a candidate for vsprintf(). :)

@lcobucci
Copy link
Member

@Majkl578 TIL, thanks 👍

@Majkl578 Majkl578 self-assigned this Sep 23, 2018
@Majkl578 Majkl578 merged commit b0e94b9 into master Sep 23, 2018
@Majkl578 Majkl578 deleted the sniff/OptimizedFunctionsWithoutUnpacking branch September 23, 2018 22:18
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👎 from me: a single array_merge(...$arrays) is still more efficient than looping and applying the function, and the optimisation is less relevant than the actual variadic call.

@Majkl578
Copy link
Contributor Author

Majkl578 commented Sep 23, 2018

array_merge() is not a specially compiled function so it's not affected.
Applies only to these functions:

		'array_key_exists',
		'array_slice',
		'boolval',
		'call_user_func',
		'call_user_func_array',
		'chr',
		'count',
		'doubleval',
		'defined',
		'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_long',
		'is_int',
		'is_integer',
		'is_null',
		'is_object',
		'is_real',
		'is_resource',
		'is_string',
		'ord',
		'strlen',
		'strval',

@Ocramius
Copy link
Member

That would still mean that (if it gets compiled), this rule still applies. Besides call_user_func(), I don't think any of these make any sense with variadic arguments

@morozov
Copy link
Member

morozov commented Sep 23, 2018

If it gets compiled, it’ll make sense to whitelist it.

@Majkl578
Copy link
Contributor Author

That would still mean that (if it gets compiled), this rule still applies.

I don't understand? This sniff will only report usage of ... for functions from the list above, because unpacking defeats VM optimization. You are free to use variadics with any other function.

I don't think any of these make any sense with variadic arguments

Hopefully, in this case the rule will be harmless. 😊

@Ocramius
Copy link
Member

Ocramius commented Sep 23, 2018 via email

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

Successfully merging this pull request may close these issues.

None yet

6 participants