Skip to content

Store fiber parent observed frames on the VM stack and fix observing functions in fibers #9193

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

Merged
merged 2 commits into from
Aug 4, 2022

Conversation

bwoebi
Copy link
Member

@bwoebi bwoebi commented Jul 29, 2022

This PR addresses two/three issues:
a) There was a significant performance penalty, when some leaf function was observed, deep in the stack.
b) The current_observed_frame is carried along the fiber context and thus always correct now.
c) As a side effect, we are not iterating over prev_execute_data anymore and thus, non-observed fake frames, possibly on stack, cannot have any impact on the observer anymore (especially within zend_observer_fcall_end_all).

Saving the previous observer happens now directly on the VM stack. If there is any observer, function frames are allocated an extra zval (the last temporary), which will, on observed frames, contain the previous observed frame address.

This avoids a possible significant performance penalty, when some leaf function was observed, deep in the stack.
As a side effect, we are not iterating over prev_execute_data anymore and thus, non-observed fake frames, possibly on stack, cannot have any impact on the observer anymore (especially within zend_observer_fcall_end_all).

Saving the previous observer happens now directly on the VM stack. If there is any observer, function frames are allocated an extra zval (the last temporary), which will, on observed frames, contain the previous observed frame address.
@bwoebi bwoebi force-pushed the tmp-prev-observer branch 2 times, most recently from 3e5c310 to ec99c3a Compare July 30, 2022 18:47
The current_observed_frame is carried along the fiber context and thus always correct now.

Signed-off-by: Bob Weinand <[email protected]>
@bwoebi bwoebi force-pushed the tmp-prev-observer branch from ec99c3a to dd67a6a Compare July 30, 2022 19:41
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.

The approach looks acceptable and I can't see any obvious performance or otherwise negative impact.

@bwoebi bwoebi merged commit da94baf into php:master Aug 4, 2022
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.

3 participants