Skip to content

Prefix all function calls with a backspace #75

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

Merged
merged 3 commits into from
Aug 1, 2018

Conversation

willemstuursma
Copy link
Contributor

@willemstuursma willemstuursma commented Jul 31, 2018

This prefixed all function calls with a back space.

This saves PHP some opcodes because it doesn't have to do the namespace lookup.

Secondly, PHP 7.2 introduced a dedicated opcode for in_array, but only if it is prefixed with a backspace (and the third argument is true). An opcode for get_called_class was also added to PHP 7.2.

See https://phpinternals.net/articles/optimising_internal_functions_via_new_opcode_instructions for background on the new opcodes.

Copy link
Member

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

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

Thanks, I've left a few comments inline

@@ -41,7 +41,7 @@
public function __construct($value)
{
if (!$this->isValid($value)) {
throw new \UnexpectedValueException("Value '$value' is not part of the enum " . get_called_class());
throw new \UnexpectedValueException("Value '$value' is not part of the enum " . \get_called_class());
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's necessary to optimize for error cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I've simply put a backslash before all function calls.

Copy link
Member

Choose a reason for hiding this comment

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

OK let's leave it that way. You're right it's no big deal.

$class = get_called_class();
if (!array_key_exists($class, static::$cache)) {
$class = \get_called_class();
if (!isset(static::$cache[$class])) {
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this change doesn't introduce BC breaks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. static::$cache can only be instances of array, for which isset returns the exact values as array_key_exists. The only difference between the two is their behaviour around null values.

@@ -181,7 +181,7 @@ public static function __callStatic($name, $arguments)
return new static($array[$name]);
}

throw new \BadMethodCallException("No static method or enum constant '$name' in class " . get_called_class());
throw new \BadMethodCallException("No static method or enum constant '$name' in class " . \get_called_class());
Copy link
Member

Choose a reason for hiding this comment

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

Same here, no need to optimize error cases.

@mnapoli mnapoli merged commit ca2f409 into myclabs:master Aug 1, 2018
@mnapoli
Copy link
Member

mnapoli commented Aug 1, 2018

Thanks!

@willemstuursma willemstuursma deleted the php-72-optimizations branch August 1, 2018 21:08
@willemstuursma
Copy link
Contributor Author

You're welcome!

@sebastienbarre
Copy link

I'm curious. Did somebody measure the actual performance impact of these changes?

@mnapoli
Copy link
Member

mnapoli commented Sep 15, 2019

Symfony people did a few months/years ago IIRC.

The summary I kept in mind was: it doesn't really matter, except for low level libs where it _could_make a small improvement in some applications.

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.

3 participants