Skip to content
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

Add new observer API #5857

Closed
wants to merge 57 commits into from
Closed

Conversation

morrisonlevi
Copy link
Contributor

@morrisonlevi morrisonlevi commented Jul 15, 2020

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.

ext/observer/observer.c Outdated Show resolved Hide resolved
@morrisonlevi
Copy link
Contributor Author

morrisonlevi commented Jul 20, 2020

@beberlei Can you add numbers from your benchmarking?

morrisonlevi and others added 2 commits Jul 24, 2020
Also clean up ext/observer's .end handling
Also add a wrapper to the runtime cache getter
@beberlei
Copy link
Contributor

beberlei commented Jul 25, 2020

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
Copy link
Contributor Author

morrisonlevi commented Jul 30, 2020

@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
Copy link
Contributor

SammyK commented Aug 28, 2020

@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
Copy link
Member

dstogov commented Aug 28, 2020

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

ZEND_API void zend_observer_activate(void) {
/* todo: how to size the arena? */
if (ZEND_OBSERVER_ENABLED) {
fcall_handlers_arena = zend_arena_create(1024);
}
}
Copy link
Contributor Author

@morrisonlevi morrisonlevi Aug 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dstogov Any recommendations on sizing the arena?

Copy link
Member

@dstogov dstogov Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

zend_llist_destroy(&handlers_list);
}

void zend_observe_fcall_begin(
Copy link
Contributor Author

@morrisonlevi morrisonlevi Aug 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@SammyK SammyK Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

void *observer_handlers = ZEND_OBSERVER_HANDLERS(&EX(func)->op_array);
if (!observer_handlers) {
zend_observer_fcall_install((zend_function *)&EX(func)->op_array);
observer_handlers = ZEND_OBSERVER_HANDLERS(&EX(func)->op_array);
}
ZEND_ASSERT(observer_handlers);
if (observer_handlers != ZEND_OBSERVER_NOT_OBSERVED) {
zend_observe_fcall_begin(observer_handlers, execute_data);
}
Copy link
Contributor Author

@morrisonlevi morrisonlevi Aug 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@SammyK SammyK Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@SammyK
Copy link
Contributor

SammyK commented Aug 31, 2020

@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
Copy link
Member

carusogabriel commented Aug 31, 2020

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

@dstogov
Copy link
Member

dstogov commented Aug 31, 2020

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

@SammyK
Copy link
Contributor

SammyK commented Sep 1, 2020

@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
Copy link
Member

dstogov commented Sep 1, 2020

@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
Copy link
Member

carusogabriel commented Sep 1, 2020

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

@morrisonlevi
Copy link
Contributor Author

morrisonlevi commented Sep 1, 2020

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.

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

Successfully merging this pull request may close these issues.

None yet

9 participants