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

faster TypeCombinator #747

Merged
merged 2 commits into from Oct 28, 2021
Merged

faster TypeCombinator #747

merged 2 commits into from Oct 28, 2021

Conversation

voku
Copy link
Contributor

@voku voku commented Oct 27, 2021

-> ~18% less "count()" calls in my first small tests
@@ -391,7 +391,7 @@ public static function union(Type ...$types): Type
unset($tempTypes[$i]);
}

if (count($tempTypes) === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should replace count($xy) === 0 with $xy === [] all over the codebase via a cs fixing rule?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the same is true for count($xy) !== 0 and $xy !== []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should replace count($xy) === 0 with $xy === [] all over the codebase via a cs fixing rule?

At work I always write "count($x) === 0" because its more readable, but here the code will be called very very often and it seems to make a difference if we call it 100000x so I don't know if we should change it everywhere? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no significant performance gain between them.

See: symfony/symfony#43779

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, I also tested it and had a quick research and count() is O(1) since the number of elements is stored in the
HashTable. So, the performance benefit in this PR was maybe from not interacting with the array at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly

@staabm
Copy link
Contributor

staabm commented Oct 28, 2021

please try to add a use function count; on the top of the class, so PHP opcache feature can fully optimize this code.

I don't have a codebase where count() is in the profiles, so I cannot check this assumption myself.

@voku
Copy link
Contributor Author

voku commented Oct 28, 2021

please try to add a use function count; on the top of the class, so PHP opcache feature can fully optimize this code.

I don't have a codebase where count() is in the profiles, so I cannot check this assumption myself.

Isn't there a fixer for it? 🤔

@staabm
Copy link
Contributor

staabm commented Oct 28, 2021

Isn't there a fixer for it?

Yep in php-cs-fixer it is.

Phpstan uses slevomat and I dont know its supported with it in the same fashion

@voku
Copy link
Contributor Author

voku commented Oct 28, 2021

Isn't there a fixer for it?

Yep in php-cs-fixer it is.

Phpstan uses slevomat and I dont know its supported with it in the same fashion

I think this Sniff will automatically fix it:

SlevomatCodingStandard.PHP.OptimizedFunctionsWithoutUnpacking

@ondrejmirtes should we use it? 🤔

@ondrejmirtes
Copy link
Member

Slevomat CS definitely has a sniff to enforce using all functions, but I don't like that codestyle and I think it has negligible impact on performance. I'd accept a change that makes the PHAR compiler do it (https://github.com/phpstan/phpstan-src/tree/master/compiler) because that's where I think it belongs, but I don't want it in phpstan-src.

@ondrejmirtes ondrejmirtes merged commit 094ff78 into phpstan:master Oct 28, 2021
@ondrejmirtes
Copy link
Member

Thank you!

@voku
Copy link
Contributor Author

voku commented Oct 28, 2021

@staabm @ondrejmirtes one maybe stupid question : why can't php optimize count call in condition like "count === 1" 🤔 ... we only need to count 0,1,break but I think php will count the full array, or?

@ondrejmirtes
Copy link
Member

@voku It probably could, raise that on https://bugs.php.net/ :)

@ondrejmirtes
Copy link
Member

But maybe not, you have to think of edgecases like Countable implementations...

@voku
Copy link
Contributor Author

voku commented Oct 28, 2021

But maybe not, you have to think of edgecases like Countable implementations...

Maybe it can be optimised for "normal" arrays?

https://bugs.php.net/bug.php?id=81561

@staabm
Copy link
Contributor

staabm commented Oct 28, 2021

within namespaced code, php can optimize count calls via opcache only when its either fully-qualified \count(..) or when you explictly use a use function count; at the top

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