Skip to content

Cache method overrides of ArrayAccess in zend_class_entry #7706

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Cache method overrides of ArrayAccess in zend_class_entry #7706

wants to merge 1 commit into from

Conversation

TysonAndre
Copy link
Contributor

Previously, code such as subclasses of SplFixedArray would check for method
overrides when instantiating the objects.

This optimization was mentioned as a followup to GH-6552

@TysonAndre TysonAndre requested a review from nikic December 2, 2021 15:15
TysonAndre
TysonAndre commented Dec 2, 2021
View reviewed changes
Zend/zend_object_handlers.c Outdated
@@ -908,7 +908,9 @@ ZEND_API zval *zend_std_read_dimension(zend_object *object, zval *offset, int ty
zend_class_entry *ce = object->ce;
zval tmp_offset;

if (EXPECTED(zend_class_implements_interface(ce, zend_ce_arrayaccess) != 0)) {
/* Assume arrayaccess_funcs_ptr is set if (and only if) the class implements zend_ce_arrayaccess */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume this would have been set on a class by the time an object was instantiated, even with late static binding or opcache, but am not 100% sure of this?

Copy link
Member

Choose a reason for hiding this comment

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

It's okay to assume this.

nikic
nikic reviewed Dec 3, 2021
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

This generally looks fine -- but I'm wondering if there is some kind of more generic solution to this. I mean, we also need this kind of functionality for methods in other classes, and currently these are handled by computing them on construction and storing them in the object, which is obviously not ideal.

It would be nice if there was a way to store custom extra data in classes without having to hardcode a few fixed cases, but I haven't really thought about how such a design would look like.

Zend/zend_object_handlers.c Outdated
@@ -908,7 +908,9 @@ ZEND_API zval *zend_std_read_dimension(zend_object *object, zval *offset, int ty
zend_class_entry *ce = object->ce;
zval tmp_offset;

if (EXPECTED(zend_class_implements_interface(ce, zend_ce_arrayaccess) != 0)) {
/* Assume arrayaccess_funcs_ptr is set if (and only if) the class implements zend_ce_arrayaccess */
Copy link
Member

Choose a reason for hiding this comment

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

It's okay to assume this.

@TysonAndre
Copy link
Contributor Author

This generally looks fine -- but I'm wondering if there is some kind of more generic solution to this. I mean, we also need this kind of functionality for methods in other classes, and currently these are handled by computing them on construction and storing them in the object, which is obviously not ideal.

It would be nice if there was a way to store custom extra data in classes without having to hardcode a few fixed cases, but I haven't really thought about how such a design would look like.

Maybe a void* that would be freed if set for ZEND_INTERNAL_CLASS, possibly in addition to a callback on zend_class_entry * if something more complicated than free is needed (or if data user-defined classes also needed to be handled). Getting it to work with opcache and the file cache would complicate that and require yet another callback type -- there might be edge cases with opcache/preloading and inheritance (I'm not familiar with that part of the code)

  • This assumes user-defined classes use the ephemeral zend_arena_alloc
  • This assumes nested data isn't needed (e.g. HashTables)

For ArrayAccess in general, there's still the tiny speedup for the array shorthand in the engine in zend_std_read_dimension, though I hadn't benchmarked it and expect function call overhead to be much larger

@TysonAndre
Cache method overrides of ArrayAccess in zend_class_entry
2767cd0 
Previously, code such as subclasses of SplFixedArray would check for method
overrides when instantiating the objects.

This optimization was mentioned as a followup to phpGH-6552
@TysonAndre TysonAndre force-pushed the arrayaccess_funcs_ptr branch from 7a78db9 to 2767cd0 Compare December 4, 2021 02:51
@TysonAndre
Copy link
Contributor Author

Closed in 024d5f4

@TysonAndre TysonAndre closed this Dec 4, 2021
@TysonAndre TysonAndre deleted the arrayaccess_funcs_ptr branch December 4, 2021 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TysonAndre @nikic