Skip to content
This repository was archived by the owner on Oct 6, 2023. It is now read-only.

Support PHP 8.0 Observer API - enabling JIT support #96

Merged
merged 8 commits into from
Dec 16, 2020

Conversation

beberlei
Copy link
Contributor

@beberlei beberlei commented Apr 14, 2020

With this PR Tideways XHProf will support PHP 8 through the new Observer API: php/php-src#5857

@SammyK
Copy link

SammyK commented Apr 18, 2020

@beberlei This looks fantastic! Did you run into any issues with implementation or side effects?

I'm not sure there will be much of a performance gain with all function calls instrumented, but we haven't really focused on that aspect yet. We'll certainly need help from Niki, Dmitry, Joe, or someone on that part.

But even if we can't get much of a performance boost with the new API when instrumenting all the things, the benefit of not having to hook zend_execute_* to artificially limit the stack and cause the compiler to emit all DO_FCALL's would still be a win IMO. :)

@beberlei
Copy link
Contributor Author

@SammyK I believe the positive thing we can draw from this is that the new instrumentation API doesn't seem to make performance worse for the case where everything is instrumented. So combined with the benefit of getting rid of zend_execute_ex, it is a win-win. I wasn't even looking to get a perf benefit out of it. Profiling/Tracing PHP can be done "fast enough" already.

For the case where only a fraction is instrumented, this API by design should be faster than an API that checks explicit function instrumentation on every call during zend_execute_ex by hash lookup.

@beberlei
Copy link
Contributor Author

@SammyK @morrisonlevi just realized it doesn't work with internal functions yet. This test here 0ca7b91 becomes only "main()==>double".

@SammyK
Copy link

SammyK commented Apr 19, 2020

@beberlei When tracer_observer_instrument() is run for internal functions, it happens in zend_fetch_function() which is called before the tideways_xhprof_enable() call. So TXRG(enabled) will be 0 for all internal function calls in this case.

@morrisonlevi This brings up an important distinction regarding order of operations between userland and internal functions. Although internal functions are checked early by design, I'm sure other extensions will run into this same issue that Benjamin brought up. At any rate, it's a use case that we'll need to consider with the API.

@SammyK
Copy link

SammyK commented May 3, 2020

@beberlei FYI I've merged @morrisonlevi's PR and updated the observer extension with the new API changes.

EDIT: And also added return_value to the end handler.

@beberlei beberlei mentioned this pull request Nov 26, 2020
@beberlei beberlei changed the title [PoC] Adapt to experimental zend_instrument API proposed for PHP 8 Support PHP 8.0 Observer API - enabling JIT support Dec 3, 2020
@andypost
Copy link
Contributor

andypost commented Dec 3, 2020

Changes looks promising! Is there suggestion about how to test it with jit enabled?
Faced with the issue in xhprof longxinH/xhprof#51

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants