perf(system): resolve the executing frame from execute_data instead of debug_backtrace in OpCodeHook - #199
Conversation
…Hook OpCodeHook::handle() captured a debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 10) on EVERY execution of a hooked opcode, only to answer "is z-engine's own code running this opcode?". Hooking a hot opcode (ASSIGN, DO_FCALL, EXT_STMT) paid a full stack walk per operation. The answer is already in the callback argument: the zend_execute_data* IS the frame that executes the opcode, and its zend_function carries the class scope that debug_backtrace() would have reported as the frame's "class". The filter now reads that scope off the frame - two struct reads and one zend_string materialization, no stack walk, no allocation per frame. Frame resolution goes through the owning wrappers, not raw CData at the call site: ExecutionData::getFunctionScopeName() (frame owner) delegates the entry read to ReflectionFunction::getScopeNameOf() (zend_function owner). The latter is a pointer-level static on purpose - ReflectionFunction::fromCData() initializes the native reflection state, which does a function-table lookup and throws (with its own trace capture) for method entries, i.e. exactly the cost being removed. Preserved semantics, case by case: - method/closure frame: scope name is the declaring/bound class, so a "ZEngine..." prefix still dispatches to the default handler - including frames of the test suite itself, whose namespace is also ZEngine\ (autoload-dev maps it to tests/), which is why OpCodeHookTest compiles its probes as global functions; - global function frame: no scope, previously reported by debug_backtrace() as a frame without a 'class' key -> empty class -> user handler runs. Now scope is NULL -> user handler runs; - main-script/pseudo frame: produced no backtrace frame at all -> empty class -> user handler ran. Its op_array carries no scope either -> user handler runs; - no function entry / unresolvable scope (trampolines, internal entries without a scope): conservatively treated as non-ZEngine, i.e. the user handler runs, which is the outcome the backtrace variant produced for those frames. Stacked hooks no longer need frame skipping. The old loop skipped frames whose class was OpCodeHook itself because a hook delegating to its predecessor via $this->originalHandler pushed its own handle() frame between the executing frame and the backtrace root. Delegation passes the SAME execute_data pointer down the chain, so every hook in the chain resolves the identical executing frame directly - there is nothing between them to skip. As a side effect the reentrancy guard got stricter in the right direction: an opcode genuinely executed inside OpCodeHook::handle() now resolves to OpCodeHook's own scope and dispatches, instead of being skipped over in search of a deeper frame. Also in this file: Closure::fromCallable([$this, 'handle']) -> $this->handle(...) (the style AbstractModule already uses), and the now-dead BACKTRACE_LIMIT constant is replaced by the named ENGINE_SCOPE_PREFIX it always served. docs/self-debugging.md described the per-hit backtrace filter as a cost driver and is updated accordingly. No phpstan-baseline entries covered the removed code, so none were pruned; phpstan level max and php-cs-fixer stay clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG
Re-bases the CI signal on the true merged state: 8.4 gained the exception-factory classes (#201) that also touch OpCodeHook's install-time guards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG
| $userEntry = $entry; | ||
| $scope = $userEntry->common->scope; | ||
| } | ||
| if ($scope === null) { |
There was a problem hiding this comment.
Use hear and upper the null-safe operator
| * | ||
| * @internal hot-path accessor for engine callbacks (see OpCodeHook::handle()) | ||
| */ | ||
| public static function getScopeNameOf(object $functionEntry): ?string |
There was a problem hiding this comment.
I don’t like the raw passing of structure as argument - it is bad design. All work around C structure should be isolated. We can have overriden method getClosureScopeClass() implementing wrapping around scope.
| * that run on every single opcode (see OpCodeHook::handle()). The scope name is | ||
| * exactly what debug_backtrace() reports as the frame's "class". | ||
| */ | ||
| public function getFunctionScopeName(): ?string |
There was a problem hiding this comment.
I don’t like the delegation, drop the function or implement the ExecutionData->getScopeClass() if not present in API (similar names should be checked)
| return null; | ||
| } | ||
|
|
||
| return ReflectionFunction::getScopeNameOf($rawFunction); |
There was a problem hiding this comment.
This delegation is bad, which looks ugly
…pers Per maintainer review on #199: no raw zend_function crossing an API boundary and no static entry helpers. ReflectionFunction now overrides the native getClosureScopeClass(), wrapping the scope it already owns; ExecutionData::getScopeClass() connects the frame to its entry with a null-safe chain; OpCodeHook reads the scope name off the returned wrapper. getScopeNameOf()/getFunctionScopeName() are gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG
|
All four review comments addressed in 06f7ec9 — the delegation and the raw-struct static are gone:
One honest trade to be aware of: the filter now materializes the reflection wrappers per hooked-opcode dispatch ( phpstan (level max) and php-cs-fixer re-verified clean; branch already carries the 8.4-tip merge. Generated by Claude Code |
Makes the CI signal concrete on the exact combination the merge preview tests: this branch's dedup plus the merged wave (frame-scope resolution, sizeOfType migration, exception factories). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG
The defect
OpCodeHook::handle()captured adebug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 10)and scanned its frames on every execution of a hooked opcode, only to answer one question: is z-engine's own code running this opcode? Hooking a hot opcode (ASSIGN,DO_FCALL, orEXT_STMTwithCOMPILE_EXTENDED_STMT) paid a full stack walk per operation — the single largest per-dispatch cost in the library.The change
The answer is already in the callback argument. The
zend_execute_data*handed to the handler is the frame executing the opcode, and itszend_functioncarries the class scope thatdebug_backtrace()would have reported as that frame's"class"(the engine fills the backtraceclasskey fromfunc->common.scope->name). The filter now reads the scope straight off the frame: two struct reads plus onezend_stringmaterialization, no stack walk, no per-frame allocation.The
ExecutionDatais built once and reused for both the filter and the user handler call.Frame resolution goes through the owning wrappers, never a raw
CDatapoke at the call site (AGENTS.md):ExecutionData::getFunctionScopeName()— frame owner; readsexecute_data.funcand hands the entry to its owner.ReflectionFunction::getScopeNameOf()—zend_functionowner; pointer-level static that resolvesscope->namefor both user and internal entries.getScopeNameOf()is deliberately a pointer-level static rather than a call throughExecutionData::getFunctionEntry():ReflectionFunction::fromCData()initializes native reflection state, which performs a function-table lookup and throws aReflectionException(capturing its own trace) for method entries — i.e. it would have reintroduced exactly the cost this PR removes, on the hottest path.Preserved semantics
class)ZEngine\*ZEngine\...→DISPATCHZEngine\...→DISPATCHZEngine\also maps totests/via autoload-dev)DISPATCHDISPATCHeval-compiled probes inOpCodeHookTest)classkey →''→ user handlerNULL→ user handler''→ user handlerOpCodeHookTest::compileProbe()documents the contract this preserves: probes must be global functions, "because opcodes executed inside ZEngine classes bypass user handlers by design". Both the class-prefix exclusion and the global-function pass-through are unchanged.Why stacked hooks no longer need frame skipping
The old loop skipped frames whose class was
OpCodeHookitself: when hook B delegates to its predecessor A through$this->originalHandler, B'shandle()frame sits between the executing frame and the backtrace root, so the naive "frame 1" would have beenOpCodeHook::handlerather than the code under instrumentation.Reading the frame from
execute_datamakes that structural: delegation passes the samezend_execute_data*pointer down the chain, so every hook in the chain resolves the identical executing frame directly — there is nothing between them to skip. The chaining tests (testSecondHandlerChainsOnDispatch,testUninstallTopReactivatesPreviousHandler) exercise exactly this path.One deliberate behavioral tightening in the right direction: an opcode genuinely executed inside
OpCodeHook::handle()now resolves toOpCodeHook's own scope and dispatches, instead of being skipped over in search of a deeper (possibly non-ZEngine) frame — a strictly safer reentrancy guard.Also in this PR
Closure::fromCallable([$this, 'handle'])→$this->handle(...)ininstall()andrefreshTrampoline(), matching the first-class-callable styleAbstractModulealready uses.BACKTRACE_LIMITconstant is gone, replaced by the namedENGINE_SCOPE_PREFIXit implicitly served.docs/self-debugging.mddescribed the per-hitdebug_backtrace(…, 10)filter as a cost driver of statement-granular instrumentation; that paragraph is updated to the new mechanism.phpstan-baseline.neonentries covered the removed code (none referenceOpCodeHook.php), so nothing was pruned and nothing was added.Validation
Tests could not be run locally and CI must provide that signal. This container runs PHP 8.5.9 while this branch targets PHP 8.4; per the non-negotiable version rule in AGENTS.md, running z-engine code (anything reaching
Core::init()) against a mismatched PHP minor reads engine structs at the wrong offsets, so no test was executed here.Static gates, run against this branch's tree, both clean:
vendor/bin/phpstan analyse→ [OK] No errors (level max)PHP_CS_FIXER_IGNORE_ENV=1 vendor/bin/php-cs-fixer fix --dry-run→ 0 of 296 files to fixReviewer attention is most valuable on the opcode-hook suites:
tests/System/Hook/OpCodeHookTest.php,tests/System/ExecutorExceptionSuppressTest.php, and thetests/Memory/scenarios/constant-table-churn.phpscenario.🤖 Generated with Claude Code
https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG
Generated by Claude Code