Make internal run_time_cache a persistent allocation by bwoebi · Pull Request #15040 · php/php-src

bwoebi · 2024-07-20T16:01:35Z

We also add zend_map_ptr_static, so that we do not incur the overhead of constantly recreating the internal run_time_cache pointers on each request when observers are enabled.

Additionally it saves a bit of zeroing of the map_ptrs on each request, independently of whether observers are actually enabled or not, given that they're below the zend_map_ptr_static_size threshold now.

Comparing instruction counts, it shows about 0.1% improvement without observers.

This mechanism might be extended for mutable_data of internal classes too. And possibly also for preloaded code.

morrisonlevi · 2024-07-22T19:27:28Z

What's the purpose of the uintptr_t casts being moved to intptr_t?

Negative offsets with respect to CG(map_ptr_base). (Still positive compared to map_ptr_real_base).
But having negative offsets for the static ones allows allocating positive offsets for non-static map_ptrs during startup as well.

morrisonlevi · 2024-07-22T19:35:00Z

Any reason these are mutable globals as opposed to thread-locals?

That's the point - they're supposed to be allocated fully during startup. And startup is the same across all threads.

iluuu1994

I don't see any issues otherwise.

arnaud-lb

Nice!

As the internal runtime cache is never re-allocated after startup due to this change, it looks like that we could store a true pointer in ->run_time_cache instead of an offset, at least in non-ZTS builds?

bwoebi · 2024-07-23T14:32:52Z

@arnaud-lb Yes, in NTS that's possible.

staabm · 2024-07-23T14:37:53Z

is this change meant to improve performance? if so, can the diff be measured?

dstogov · 2024-07-24T09:40:07Z

I have benchmarked this patch on Symfony Demo with callgrind (with and without observer), using the following commands:

ZEND_DONT_UNLOAD_MODULES=1 valgrind --tool=callgrind --separate-recs=1 --dump-instr=yes --cache-sim=no sapi/cgi/php-cgi -d opcache.jit=0 -T2,100 /home/dmitry/php/community_tests/symfony_demo/public/index.php > /dev/null
ZEND_DONT_UNLOAD_MODULES=1 valgrind --tool=callgrind --separate-recs=1 --dump-instr=yes --cache-sim=no sapi/cgi/php-cgi -d opcache.jit=0 -d zend_test.observer.enabled=1 -d zend_test.observer.show_output=0 -T2,100 /home/dmitry/php/community_tests/symfony_demo/public/index.php > /dev/null

master without observer:  860,901,434
master with observer:     944,021,946 (observer makes ~9% slowdown)
patched without observer: 859,001,591 (patch makes ~0.2% improvement)
patched with observer:    930,449,148 (patch makes ~1.5% improvement, now observer makes ~8% slowdown)

So the patch makes some performance improvement for observer (eliminates call to zend_init_internal_run_time_cache() on each request) and "as a side effect" also makes slight improvement for normal execution (keeps "static" part of map_ptr space unchanged between requests).

It would be great to saw this explanation in PR comments...

dstogov · 2024-07-24T09:53:00Z

Why do we need this?
Why this is done only for non-ZTS build?
Will we have any problems with ZTS build?

Please use ZEND_MAP_PTR_INIT instead of ZEND_MAP_PTR() on left size.

I'm using MAP_PTR_INIT for everything where I assign a value, but in this case I want to copy whatever binary data is, thus I found using ZEND_MAP_PTR directly more fitting and symmetric with the right side.

Also, only need it in NTS, as in ZTS this uses a proper map_ptr which is set when the function is created and thus existing here.
In NTS the pointer is initialized later in the startup sequence and thus needs to be copied back.

dstogov · 2024-07-24T10:01:17Z

Can you explain why ZTS makes difference?

We need per-thread pointers for ZTS (run-time cache is local to each request). But for NTS we can, as a slight optimization, just skip the indirect lookup via CG(map_ptr_base).

Probably worth adding a comment here in code.

dstogov

The patch looks more or less good (I may miss some edge cases in implementation)
Please, answer my questions and make minor clean up.

You don't see any problems for Windows, where different workers have different copies of internal functions?

bwoebi · 2024-07-24T11:13:29Z

@dstogov This is not problematic on Windows, as this is fully local to each process. There's zero interaction with shared memory here.

We also add zend_map_ptr_static, so that we do not incur the overhead of constantly recreating the internal run_time_cache pointers on each request. This mechanism might be extended for mutable_data of internal classes too.

opcache persists CE-cache map_ptr offsets into SHM-interned class-name strings (zend_accel_get_class_name_map_ptr()), allocating them from the persisting process's CG(map_ptr_last). That numbering is process-local, and php-fpm de-synchronizes it across pools of one master: pool-level php_admin_value[extension] lines are loaded post-fork, and each internal function of such an extension claims a dynamic map_ptr slot at MINIT. A CE-cache offset baked by a pool without the extension then lands, in pools with the extension, inside the band of slots owned by internal-function run-time caches. When an fcall observer is registered, zend_init_internal_run_time_cache() fills every such slot with a zero-initialized arena slice each request, and zend_lookup_class_ex()'s CE-cache fast path - whose only guard is slot < CG(map_ptr_last) - returns that slice as a zend_class_entry *. The next static method call does zend_hash_find() on a zeroed function_table and segfaults at 4 * zend_string_hash(method_name). Backport the static map_ptr region from PHP 8.4, where this class of bug is fixed by design: - 25d7616 (phpGH-15040): internal-function run-time caches are allocated from a separate static region via ZEND_MAP_PTR_NEW_STATIC() instead of the per-process dynamic counter, so pool-local extension MINIT no longer advances CG(map_ptr_last) and SHM-persisted CE-cache offsets stay consistent across pools. - The map_ptr_static preload handling from 53fa98e (phpGH-17835): opcache's preload_load() re-allocates the map_ptr base itself and must account for the static region, otherwise the heap is corrupted under opcache.preload. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

github-actions Bot added the Category: Engine label Jul 20, 2024

bwoebi force-pushed the optimize-internal-rt-cache branch 5 times, most recently from 522803f to 818f6d5 Compare July 22, 2024 02:35

bwoebi marked this pull request as ready for review July 22, 2024 15:47

bwoebi requested review from bukka, dstogov and iluuu1994 as code owners July 22, 2024 15:47

morrisonlevi reviewed Jul 22, 2024

View reviewed changes

iluuu1994 reviewed Jul 23, 2024

View reviewed changes

Comment thread Zend/zend.c Outdated

Comment thread Zend/zend.c Outdated

Comment thread Zend/zend.c Outdated

Comment thread Zend/zend.c Outdated

Comment thread Zend/zend.c Outdated

Comment thread Zend/zend.c Outdated

Comment thread Zend/zend.c Outdated

arnaud-lb reviewed Jul 23, 2024

View reviewed changes

Comment thread Zend/zend_types.h Outdated

bwoebi force-pushed the optimize-internal-rt-cache branch 2 times, most recently from 07a4cce to 477314f Compare July 24, 2024 01:11

bwoebi requested a review from ndossche as a code owner July 24, 2024 01:11

github-actions Bot added Extension: ffi Extension: xmlreader labels Jul 24, 2024

bwoebi force-pushed the optimize-internal-rt-cache branch 3 times, most recently from a28a5f1 to 671d06d Compare July 24, 2024 02:39

github-actions Bot added the Extension: opcache label Jul 24, 2024

dstogov reviewed Jul 24, 2024

View reviewed changes

Make internal run_time_cache a persistent allocation

10a4c24

We also add zend_map_ptr_static, so that we do not incur the overhead of constantly recreating the internal run_time_cache pointers on each request. This mechanism might be extended for mutable_data of internal classes too.

bwoebi and others added 2 commits September 7, 2024 01:02

Ah there's the bug

cc5fef0

Apply code review suggestions

87f038e

bwoebi force-pushed the optimize-internal-rt-cache branch from 671d06d to 87f038e Compare September 6, 2024 23:02

bwoebi merged commit 25d7616 into php:master Sep 6, 2024

nyrzhun mentioned this pull request Aug 18, 2026

PHP-FPM SIGSEGV with OPcache, an observer, and an extension loaded per pool #23355

Open

msfukui mentioned this pull request Sep 9, 2026

PHP-FPM worker SIGSEGV: run_time_cache map_ptr offset beyond CG(map_ptr_last) when a class is reached through the CE cache #23637

Open

Uh oh!

Conversation

bwoebi commented Jul 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arnaud-lb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bwoebi commented Jul 23, 2024

Uh oh!

staabm commented Jul 23, 2024

Uh oh!

dstogov commented Jul 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

bwoebi commented Jul 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bwoebi commented Jul 20, 2024 •

edited

Loading

bwoebi Jul 22, 2024 •

edited

Loading

bwoebi Jul 24, 2024 •

edited

Loading