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

Use spl_object_id() instead of spl_object_hash() #413

Open
wants to merge 1 commit into
base: 4.1.x
Choose a base branch
from

Conversation

rosier
Copy link

@rosier rosier commented Jan 25, 2025

Small performance tweak.

@greg0ire
Copy link
Member

Please retarget to 4.1.x

@rosier rosier changed the base branch from 3.4.x to 4.1.x January 27, 2025 19:43
@SenseException
Copy link
Member

SenseException commented Jan 27, 2025

PHP docs:

This function returns a unique identifier for the object. The object id is unique for the lifetime of the object. Once the object is destroyed, its id may be reused for other objects. This behavior is similar to spl_object_hash().

The behavior seems to be the same, but what are the performance gains by this, @rosier? Did you try some performance tests

@derrabus
Copy link
Member

I wonder if we should use an SplObjectStorage instead of an array, so we don't need to care about object hashing anymore.

@rosier
Copy link
Author

rosier commented Jan 28, 2025

The behavior seems to be the same, but what are the performance gains by this, @rosier? Did you try some performance tests

I used the benchmark from

@rosier
Copy link
Author

rosier commented Jan 28, 2025

I wonder if we should use an SplObjectStorage instead of an array, so we don't need to care about object hashing anymore.

An array + spl_object_id also outperforms SplObjectStorage

https://3v4l.org/CZjfW#v8.4.3
https://3v4l.org/CZjfW#v8.3.16
https://3v4l.org/CZjfW#v8.2.27
https://3v4l.org/CZjfW#v8.1.31

@SenseException
Copy link
Member

As far as I remember was SplObjectStorage dropped in the past because it wasn't as performant as the current array solution.

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.

4 participants