Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork131
Improve performance by usingspl_object_id() on PHP 7.2+#267
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
it seems cause error in test: |
I see, |
CI error seems unrelatedhttps://github.com/reactphp/event-loop/actions/runs/6026916701/job/16350927220?pr=267#step:4:88 |
Hey@samsonasik, thanks for your contribution 👍 I can see from your benchmark that using the
Regarding the CI error, the pecl/uv extension received a new v0.3.0 in June which is only compatible with 8+ (for reference see:https://pecl.php.net/package/uv) . I'll have a look at this 👍 |
Hi, we are using event-loop at rector, we use spl_object_id and it seems improve the performance, see related PR:. |
0221e0d to5897873Compare@SimonFrings rebased 👍 |
All green 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@samsonasik Thank you for looking into this and filing this PR! 👍
The changes LGTM and while I do not expect any significant changes in real-world use cases, I can confirm this does indeed improve performance slightly in some synthetic benchmarks:
$time php examples/92-benchmark-timers.php 1000000# new: ~3.2s# old: ~4.1s
Interestingly, I've also applied somewhat similar changes withreactphp/http#467 in the past.
May I ask you to squash your changes into a single commit so we can go ahead with this one?
spl_object_id() on PHP 7.2+5897873 to4a8f6afCompare@clue I've squashed the changes into single commit 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@samsonasik Thanks for the update, changes LGTM! Keep it up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@samsonasik Much appreciate putting the time and effort in this improvement 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice work 👍
spl_object_id()is exists since php 7.2, I thinkspl_object_id()can be used when php version >= 7.2Refs:
Same implementation in reactphp/http:
https://github.com/reactphp/http/blob/c6321978bfb82de979fe4dc28eb0c08bc34937ad/src/Io/RequestHeaderParser.php#L165
Benchmark
Beforecall in loop took: 13.639ms
https://3v4l.org/4rUL4
Aftercall in loop took: 4.835ms
https://3v4l.org/jKtO6