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 using spl_object_id() on PHP 7.2+ #267

Merged
merged 1 commit into from Oct 25, 2023

Conversation

samsonasik
Copy link
Contributor

spl_object_id() is exists since php 7.2, I think spl_object_id() can be used when php version >= 7.2

Refs:

Same implementation in reactphp/http:

https://github.com/reactphp/http/blob/c6321978bfb82de979fe4dc28eb0c08bc34937ad/src/Io/RequestHeaderParser.php#L165

Benchmark

Before call in loop took: 13.639ms

https://3v4l.org/4rUL4

After call in loop took: 4.835ms

https://3v4l.org/jKtO6

@samsonasik
Copy link
Contributor Author

it seems cause error in test:

➜  event-loop git:(use-spl-object-id) vendor/bin/phpunit tests/Timer/TimersTest.php
PHPUnit 9.6.11 by Sebastian Bergmann and contributors.

.F                                                                  2 / 2 (100%)

Time: 00:01.009, Memory: 6.00 MB

There was 1 failure:

1) React\Tests\EventLoop\Timer\TimersTest::testContains
Failed asserting that false is true.

/Users/samsonasik/www/event-loop/tests/Timer/TimersTest.php:37

FAILURES!
Tests: 2, Assertions: 2, Failures: 1.

@samsonasik
Copy link
Contributor Author

I see, $id need to be filled on contains() method 0221e0d

@samsonasik
Copy link
Contributor Author

@SimonFrings
Copy link
Member

Hey @samsonasik, thanks for your contribution 👍

I can see from your benchmark that using the spl_object_id() seems to be way more efficient than spl_object_hash(). I'm curious if this also offers some performance improvements for this project, do you have some benchmarks using ReactPHP?

CI error seems unrelated

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 👍

@samsonasik
Copy link
Contributor Author

@samsonasik
Copy link
Contributor Author

@SimonFrings rebased 👍

@samsonasik
Copy link
Contributor Author

All green 🎉

@clue clue added this to the v1.5.0 milestone Oct 21, 2023
Copy link
Member

@clue clue left a 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 with reactphp/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? :shipit:

@clue clue changed the title [Performance] Use spl_object_id() when possible Improve performance by using spl_object_id() on PHP 7.2+ Oct 21, 2023
@samsonasik
Copy link
Contributor Author

@clue I've squashed the changes into single commit 👍

Copy link
Member

@clue clue left a 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! :shipit:

Copy link
Member

@WyriHaximus WyriHaximus left a 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 👍

Copy link
Member

@SimonFrings SimonFrings left a comment

Choose a reason for hiding this comment

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

Nice work 👍

@SimonFrings SimonFrings merged commit 67f4642 into reactphp:1.x Oct 25, 2023
29 checks passed
@samsonasik samsonasik deleted the use-spl-object-id branch October 25, 2023 07:53
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.

None yet

4 participants