Add new observer API

[morrisonlevi]

This supersedes PR #5582. It incorporates some of the design feedback such as:

• Dropping support for internal functions.
• Using the run_time_cache of the zend_op_array to store the handlers.
• Uses an arena allocator for the observer handlers data and the struct hack to compact the memory.
• Minimizes overhead when the feature is unused. There may be more work to do here, but preliminary measurements show it's low.

It still uses per-function targeting, rather than a global handler that is always called. This is a design decision that should lower the overhead of observers that only instrument a handful of functions (common for application performance monitoring products) without removing generic capability to observe everything.

I changed the name from "instrumentation" to "observer" to highlight that when using this API the state generally should not be changed, only observed.

To be ready:

• Do performance testing. Thanks, beberlei.
• Add tests to zend_test extension behind a setting so it can be turned on/off.
  • The included observer extension will be dropped once zend_tests have been improved.
• Figure out what's wrong with my .end handling for require/include (sigsegv)
• Generator support.

[morrisonlevi]

@beberlei Can you add numbers from your benchmarking?

[beberlei]

The performance is very close to each other, so much that Zend/bench.php is not reliable because its either one or the other being faster on multiple iterations. Wordpress index.php with 1 post repeated 100 times is also very close at ~3,9 secs for both.

Here are the numbers of the test php-src clean (commit before this patch is introduced) vs php with this patch enabled (but no observer registered). The test setup is the same as originally done with phpng

Observer:

• Zend/bench.php 0,2423 secs
• callgrind Zend/bench.php irefs 2,262,180,545
• wordpress php-cgi -T 100: 3,923 secs
• callgrind wordpress php-cgi -T 10: 2,119,804,952 irefs

clean php-src (1c0ee68)

• Zend/bench.php 0,2525 secs
• callgrind Zend/bench.php irefs 2,237,935,192
• wordpress php-cgi -T 100 3,937 secs
• callgrind wordpress php-cgi -T 10 2,113,198,129 irefs

You can see that the patch slightly increases instructions by 1,01% on Zend/bench.php and by 0,3% on Wordpress.

Commands

/opt/php/php-src/bin/php Zend/bench.php >> /tmp/results
valgrind --tool=callgrind opt/php/php-src/bin/php Zend/bench.php
/opt/php/php-src/bin/php-cgi -T 100 -c /opt/php/php-src/etc index.php > /dev/null
valgrind --tool=callgrind /opt/php/php-src/bin/php-cgi -T 10 -c /opt/php/php-src/etc index.php > /dev/null

configure

'./configure' \
'--prefix=/opt/php/php-src' \
'--with-config-file-path=/opt/php/php-src/etc' \
'--enable-mbstring' \
'--enable-bcmath' \
'--enable-pcntl' \
'--enable-ftp' \
'--enable-exif' \
'--enable-calendar' \
'--enable-sysvmsg' \
'--enable-sysvsem' \
'--enable-sysvshm' \
'--with-curl' \
'--with-iconv' \
'--with-gmp' \
'--with-pspell' \
'--with-zlib-dir=/usr' \
'--enable-gd-jis-conv' \
'--with-openssl' \
'--with-pdo-mysql=mysqlnd' \
'--with-gettext=/usr' \
'--with-zlib=/usr' \
'--with-bz2=/usr' \
'--with-mysqli=mysqlnd' \
"$@"

php.ini

date.timezone=UTC
max_execution_time=600
memory_limit=128M
error_reporting=0
display_errors=0
log_errors=0
user_ini.filename=
realpath_cache_size=2M
cgi.check_shebang_line=0

zend_extension=opcache.so
opcache.enable=1
opcache.enable_cli=1
opcache.save_comments=0
opcache.validate_timestamps=1
opcache.revalidate_freq=60
opcache.use_cwd=1
opcache.max_accelerated_files=100000
opcache.max_wasted_percentage=5
opcache.memory_consumption=128
opcache.consistency_checks=0

opcache.jit=0
opcache.jit_buffer_size=0

[morrisonlevi]

@dstogov Do you have any concerns with this patch's implementation? According to Ben's benchmarks the overhead when not in use is acceptable. We still need to add tests and get generators working correctly, but I want to check in with you on the implementation and any remaining concerns.

[SammyK]

@dstogov

good work! now the performance overhead is almost completely eliminated when observer disabled.

Thanks! And \o/

Move LOAD_OPLINE_EX() back to the end of DO_UCALL handler.

Done.

Don't mark "observer" handler as ZEND_VM_HOT (to prevent inlining), probably better to mark them ZEND_VM_COLD

Done.

Try to disable specialization of "observer" handlers (to reduce the code size).

I refactored the observer runtime cache init and removed SPEC(OBSERVER) from all of the init handlers. This reduced the PR size to nearly half. Let me know if that is sufficient or if you would rather I also disable specialization of observer handlers.

Travis shows a lot of crashes :(

Yes. Sorry about that. I had inadvertently run all the tests in observer mode in CI after the SPEC(OBSERVER) refactor. There was also a missing NULL check in an unhappy observer path which wreaked havoc. That's all squared away now. :)

I had to add an observer check to zend_call_function to observe internal calls to userland. I don't think this has impacted the benchmarks very much, but I wanted to point that out just in case.

If an observer extension is present, JIT will be disabled. JIT support isn't something we are hoping to get supported for merge, but we might want to add support for it later.

Thanks so much for responding to us so promptly Dmitry! My schedule is completely cleared now through EOD Monday to get anything else wrapped up for merge before RC1 if possible. So let me know if there's anything else that needs to be tweaked and I'll get on it straight away. :)

[dstogov]

@SammyK I may spend 1-2 hours on Monday to check this once again (and may be improve something).
In current state (yesterday review and following commit titles), I don't see big problems any more.

[morrisonlevi]

@dstogov Any recommendations on sizing the arena?

[dstogov]

(N * 4096) should be friendly for Zend MM.

[morrisonlevi]

Do the definitions of zend_observe_fcall_begin, zend_observer_fcall_call_end_helper, and zend_observe_fcall_end need ZEND_API too? Or is it sufficient for the declaration only? I don't know Windows, which mostly seems to be the only platform this affects.

[SammyK]

I don't see a reason to export them at the moment. We can always export them later if there is a need to. That being said, we could probably remove ZEND_API from the startup/activate and shutdown/deactivate functions. Not sure those should ever be called from a linked lib.

[morrisonlevi]

Should the body here get extracted into a helper like I did for zend_observer_maybe_fcall_call_end to reduce code size when inlining? Or should the reverse be done and zend_observer_fcall_call_end_helper inline into zend_observer_maybe_fcall_call_end?

The original motivation of the helper was to reduce code size without hurting performance much; the main conditional check would inline but if it's true then the helper body isn't inlined, so it won't hurt code size. Now that these are mostly done in spec handlers, we are probably fine with more aggressive inlining, I would guess?

[SammyK]

I don't know what would be better so I don't have any strong opinions on this one.

[SammyK]

@dstogov Any issues with merging this one in today to make this part of RC1? :) Once we update a few extensions to use the new API we'll likely have a followup PR in about a month or so to wrap up any additional issues found.

[carusogabriel]

@SammyK RC1 is not this week's release, so there's no need to rush: https://wiki.php.net/todo/php80#timetable

[dstogov]

Looks good to me.
It would be good to avoid specialization for OBSERVER handlers to reduce the code size.

[SammyK]

@dstogov

It would be good to avoid specialization for OBSERVER handlers to reduce the code size.

Would this be a blocker for merge? I gave this a shot today to remove the RETVAL specialization, but I'll likely need a little help to get this working properly. Perhaps this could be knocked out in the followup PR? :)

@carusogabriel Ah yes, my mistake: Beta 3. I misread @sgolemon's comment above:

Oh, and if possible (without rushing), try to merge prior to Tuesday so that we can let beta3 bake this in for a bit before RC1 brings any noise from the attributes change.

[dstogov]

@dstogov

It would be good to avoid specialization for OBSERVER handlers to reduce the code size.

Would this be a blocker for merge? I gave this a shot today to remove the RETVAL specialization, but I'll likely need a little help to get this working properly. Perhaps this could be knocked out in the followup PR? :)

This is not a blocker. Just don't forget this.

[carusogabriel]

@SammyK No problem. I'll wait until EOD today (18:00h CEST) and tag the release.

[morrisonlevi]

I've just merged in the latest master, which had a conflict. Tests passed locally; just waiting on CI to hopefully show there were no surprises and then I will merge it in.