tideways / php-xhprof-extension

This XHProf PHP extension fork has outlived its purpose and is archived in favor of the revitalized https://github.com/longxinH/xhprof
https://tideways.com
Apache License 2.0
1.63k stars 209 forks source link

Support PHP 8.0 Observer API - enabling JIT support #96

Closed beberlei closed 3 years ago

beberlei commented 4 years ago

With this PR Tideways XHProf will support PHP 8 through the new Observer API: https://github.com/php/php-src/pull/5857

SammyK commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@SammyK @morrisonlevi just realized it doesn't work with internal functions yet. This test here https://github.com/tideways/php-xhprof-extension/pull/96/commits/0ca7b91d0f2ad18554055bac1b57ecbe321dbc96 becomes only "main()==>double".

SammyK commented 4 years ago

@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 commented 4 years ago

@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.

andypost commented 3 years ago

Changes looks promising! Is there suggestion about how to test it with jit enabled? Faced with the issue in xhprof https://github.com/longxinH/xhprof/issues/51