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

Improve performance by change count(array) to empty array [] comparison #917

Conversation

samsonasik
Copy link
Contributor

@samsonasik samsonasik commented Jan 10, 2022

@ondrejmirtes @TomasVotruba this is for increase PHPStan performance improvement by replace count($array) === 0 or count($array) > 0 to empty array comparison $array === [] or $array !== [], it utilize rector's Rector\CodingStyle\Rector\FuncCall\CountArrayToEmptyArrayComparisonRector::class.

Rector probably needed to be part of CI workflow to verify it if it is approved, @ondrejmirtes what do you think?

@samsonasik
Copy link
Contributor Author

compiler tests notice seems unrelated

@ondrejmirtes
Copy link
Member

Hi, thank you, but I doubt this has real world performance implications. Did you measure it?

@samsonasik
Copy link
Contributor Author

You can check this benchmark https://phpsandbox.io/n/polished-violet-mwjd-llhvj

Count took: 0.316ms
Compare to empty array took: 0.018ms

@TomasVotruba
Copy link
Contributor

@samsonasik Cool tool, thanks for sharing the link 👍

I tried to re-run it couple of times and got wide range of numbers:

  • count: 160-250 ms
  • compare empty array: 15-30 ms

Factor 10x for the empty array.

@ondrejmirtes
Copy link
Member

Yeah, but it's not obvious what it means for PHPStan from these benchmarks. I'll have to compare it with Blackfire.

@orklah
Copy link
Contributor

orklah commented Jan 10, 2022

Weird, I don't reproduce the same times with 3v4l at all:
https://3v4l.org/uc9iY

Maybe a difference in opcache setup?

I had to reduce the content of the array by a factor of ten, but I doubt this would have this kind of dramatic effect. I also have a different benchmark script that show similar results: https://3v4l.org/TNFbu (ie, no dramatic effect in one way or another)

But I still prefer !== [] / === [] notation !

@ondrejmirtes
Copy link
Member

I prefer the count version so I need some hard evidence this improves PHPStan performance by at least a few percents :)

@staabm
Copy link
Contributor

staabm commented Jan 10, 2022

AFAIR will php-src optimize code when you write \count() at OPCODE compile time.

using count without a prior use function or without a fully-qualified notation will not benefit from php-src compilation passes.
i.e. this PR as is improves performance when the native count function is not imported via use function.
(this only works for namespace'd code and depends on opcache beeing used)

@Dgame
Copy link

Dgame commented Jan 14, 2022

I prefer the count version so I need some hard evidence this improves PHPStan performance by at least a few percents :)

I doubt that there is one. In contrast to earlier PHP versions (I think from >=PHP 7.0) count only reads the length property of the internal buckets, so it's not as costly. I also like the === [] comparison much better, but if you compare both versions, you see almost no difference: https://3v4l.org/tF1WE

@staabm
Copy link
Contributor

staabm commented Jan 15, 2022

using count without a prior use function or without a fully-qualified notation will not benefit from php-src compilation passes.
i.e. this PR as is improves performance when the native count function is not imported via use function.
(this only works for namespace'd code and depends on opcache beeing used)

my assumption was wrong

https://twitter.com/nikita_ppv/status/1482096003721355266?s=20

@staabm
Copy link
Contributor

staabm commented Feb 1, 2022

in #747 (comment) we concluded that this change is not worth it

@samsonasik samsonasik deleted the performance-count-array-to-empty-array branch February 1, 2022 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants