Skip to content

Conversation

bwoebi
Copy link
Member

@bwoebi bwoebi commented Jul 22, 2022

Object handlers being separate from class entries is a legacy inherited from PHP 5. Today it has little benefit to keep them separate: in fact, accessing object handlers usually requires not-so-safe hacks.
While it is possible to swap handlers in a custom installed create_object handler, this mostly is tedious, as well as it requires allocating the object handlers struct at runtime, possibly caching it etc..

This allows extensions, which intend to observe other classes to install their own class handlers.
The life cycle of internal classes may now be simply observed by swapping the class handlers in post_startup stage.
The life cycle of userland classes may be observed by iterating over the new classes in zend_compile_file and zend_compile_string and then swapping their handlers.

In general, this would also be a first step in directly tying the object handlers to classes (i.e., removing the handlers field from zend_object). Especially given that I am not aware of any case where the object handlers would be different between various instances of a given class.

This PR (for now) only changes occurrences in Zend. Once the general approach is approved, I will update all other occurrences in ext/ as well.

@Girgias
Copy link
Member

Girgias commented Jul 22, 2022

The general approach makes sense to me.

I wonder if at one point it would be possible to merge the object handlers with the various zend_function* used for userland implementation of magic methods, but that looks hard TM and not immediately beneficial.

@bwoebi
Copy link
Member Author

bwoebi commented Jul 22, 2022

This probably makes some sense as part of a long-term effort to use the C stack less and have proper zend_functions for much more things. However not every handler has equivalent magic methods (e.g. free_obj).

@iluuu1994
Copy link
Member

iluuu1994 commented Jul 23, 2022

@bwoebi Does this also mean the handlers can't be changed once the class is already in shared memory? An extension like Xdebug or Blackfire might only want to override certain handlers for certain requests (e.g. when a header is set) to avoid slowdowns when the page is not being profiled. This would mean we'd likely also need to store the handlers in zend_class_mutable_data which would further complicate accessing it.

Disclaimer: I have no clue how Xdebug/Blackfire do things. They might also fall back to the std handlers internally in which case this would be less of a problem.

@bwoebi
Copy link
Member Author

bwoebi commented Jul 23, 2022

@iluuu1994 This, for now, does not impact the currently available capabilities. For now I'm just inserting the class handlers at ce creation directly into the ce, and then copying from the ce to the object at runtime. Instead of the create_object callback having to set the handlers on the object. The status quo of object handlers existing on the object is untouched. It should not impact any extensions trying to do things.

@iluuu1994
Copy link
Member

iluuu1994 commented Jul 23, 2022

@bwoebi But it would once we removed zend_object.handlers, right? Maybe @derickr and somebody from Blackfire could answer if this this would be a problem for their case. Other than that, this would add an indirection when the class isn't loaded yet with a potential cache miss but that's likely not a problem because invoking any handler will likely involve fetching the class anyway- Still, it might make sense to profile this once we make that step. Other than that it definitely makes more sense to keep this on the class, because as you said they are basically always the same for the same types of objects.

@bwoebi
Copy link
Member Author

bwoebi commented Jul 23, 2022

Yes, it would. Hence I'm projecting this to future scope as this needs more investigation / a transition period and I'm not doing it straight away.

@bwoebi
Copy link
Member Author

bwoebi commented Aug 24, 2022

I think we should avoid labeling extensions when > 5 extensions are affected... That's just funny and useless.

@bwoebi bwoebi force-pushed the default_object_handlers branch from 5e29ef5 to 205440d Compare August 24, 2022 17:03
@@ -1765,6 +1769,7 @@ static void date_register_classes(void) /* {{{ */

date_ce_period = register_class_DatePeriod(zend_ce_aggregate);
date_ce_period->create_object = date_object_new_period;
date_ce_period->default_object_handlers = &date_object_handlers_period;
Copy link
Member

Choose a reason for hiding this comment

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

As this seems to be done for every single class, wouldn't it make more sense to have the arg stubs parser/code generate do this automatically in the register_class_DatePeriod (and friends) code that it generates?

Copy link
Member Author

Choose a reason for hiding this comment

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

But how would the stubs know whether there are handlers for the object? Some classes don't need handlers and some do inherit them from their parent.

@derickr
Copy link
Member

derickr commented Aug 25, 2022

And to answer @iluuu1994 's question - it doesn't seem that @xdebug uses these object handlers for anything.

bwoebi added 3 commits August 25, 2022 17:50
Object handlers being separate from class entries is a legacy inherited from PHP 5. Today it has little benefit to keep them separate: in fact, accessing object handlers usually requires not-so-safe hacks.
While it is possible to swap handlers in a custom installed create_object handler, this mostly is tedious, as well as it requires allocating the object handlers struct at runtime, possibly caching it etc..

This allows extensions, which intend to observe other classes to install their own class handlers.
The life cycle of internal classes may now be simply observed by swapping the class handlers in post_startup stage.
The life cycle of userland classes may be observed by iterating over the new classes in zend_compile_file and zend_compile_string and then swapping their handlers.

In general, this would also be a first step in directly tying the object handlers to classes. Especially given that I am not aware of any case where the object handlers would be different between various instances of a given class.

Signed-off-by: Bob Weinand <bobwei9@hotmail.com>
memrchr has an always available equivalent under the name of zend_memrchr.

Signed-off-by: Bob Weinand <bobwei9@hotmail.com>
Signed-off-by: Bob Weinand <bobwei9@hotmail.com>
@bwoebi bwoebi force-pushed the default_object_handlers branch from 205440d to 40997de Compare August 25, 2022 15:50
@bwoebi bwoebi marked this pull request as ready for review August 26, 2022 10:12
Copy link
Member

@krakjoe krakjoe left a comment

Choose a reason for hiding this comment

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

From a hacking-the-engine-a-la-uopz-or-runkit point of view, I like this approach and think the future scope is worth working toward, so +1 from me.

@bwoebi bwoebi changed the base branch from master to PHP-8.2 August 31, 2022 14:45
@bwoebi bwoebi merged commit 94ee4f9 into php:PHP-8.2 Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment