Copy link Copy Markdown

Contributor

Copy link Copy Markdown

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.

Copy link Copy Markdown

Contributor Author

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

Copy link Copy Markdown

Contributor Author

Copy link Copy Markdown

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 👍

Merged

Copy link Copy Markdown

Contributor Author

Copy link Copy Markdown

Contributor Author

Copy link Copy Markdown

Contributor Author

All green 🎉

clue left a comment

Copy link Copy Markdown

Member

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?

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

Copy link Copy Markdown

Contributor Author

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

clue approved these changes Oct 22, 2023

clue left a comment

Copy link Copy Markdown

Member

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!

Copy link Copy Markdown

Member

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 Copy Markdown

Member

Choose a reason for hiding this comment

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

Nice work 👍

Merged