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

PHP Fixes found by Static Analysis Tool #1261

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

craiglondon
Copy link

No description provided.

@terrafrost
Copy link
Member

I like how it added the @throws docblock stuff but some of the other stuff are micro-optimizations, which I don't necessarily have a problem with, but if we're gonna go that route I'd just assume there be benchmarks. Is that something you could provide?

Thanks!

@craiglondon
Copy link
Author

I think the only commit related to Amy performance (supposed) improvements is this one.

57c87fc

I haven't done any benchmarks before but would be willing to do some given a little direction as a best tool to use? like using Blackfire and PHPunit?

@terrafrost
Copy link
Member

I've never used Blackfire lol. The way I normally benchmark stuff is just to loop over it a million times lol. And in this case I'm not sure a loop would be the most effective way to do the benchmark since I don't really expect anyone to do that stuff in a loop.

I guess two possible ideas for benchmarking:

  1. Upload a 500mb file to an SFTP server with the changes and without.
  2. Maybe try to decode a really big CRL with File/X509.php with and without the changes.

@yarkm13
Copy link

yarkm13 commented Feb 4, 2022

Why test performance of comparison operators? It's known that strict comparison is faster and benchmark proves it (approx 15% faster) https://github.com/8ctopus/php-benchmark

Typehint performance difference is negligible 0.4 ms per 100M cycles

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

Successfully merging this pull request may close these issues.

None yet

3 participants