Avoid allocating temporaries on the call frame for property hook trampolines #16287
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…polines As pointed out in php#16252 (comment)
iluuu1994
reviewed
Oct 7, 2024
| @@ -1707,6 +1707,9 @@ ZEND_API zend_function *zend_get_property_hook_trampoline( | |||
| func = (zend_function *)(uintptr_t)ecalloc(1, sizeof(zend_internal_function)); | |||
| } | |||
| func->type = ZEND_INTERNAL_FUNCTION; | |||
| /* This trampoline does not use the call_trampoline_op, so it won't reuse the call frame, | |||
| * which means we don't even need to reserve a temporary for observers. */ | |||
There was a problem hiding this comment.
I'm a bit confused by this comment. Won't this simply not use observers because of ZEND_ACC_CALL_VIA_TRAMPOLINE? See zend_observer_handler_is_unobserved().
There was a problem hiding this comment.
Yes, but see #16252 to know why this isn't always a safe assumption.
There was a problem hiding this comment.
To clarify, it's not because a function isn't observed that the call frame won't be reused by a function that becomes observed. The reuse happens when dealing with call_trampoline_op. This doesn't happen here so we don't have to account for the extra observer temporary.
There was a problem hiding this comment.
Thanks. This goes over my head at this time of day ^^ I will have a closer look tomorrow.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As pointed out in #16252 (comment)