Fix GH-15869: Stack overflow in zend_array_destroy with deeply nested arrays#21494
Fix GH-15869: Stack overflow in zend_array_destroy with deeply nested arrays#21494iliaal wants to merge 1 commit intophp:masterfrom
Conversation
dstogov
left a comment
There was a problem hiding this comment.
This would fix the stack overflow only for a single particular case.
The problem might be simple re-reproduced with $a=[$a,$a] or with objects.
Also the fix may change the order of object destructors (probably this is not critical for master branch), and may interfere with GC.
I'm not sure if we should accept this incomplete fix.
The complete fix would require maintaining a separate destruction queue, but this may introduce different troubles.
|
Updated the branch to address the multi-branch case. I added a destroy stack alongside the tail-call. On each point:
The real gap was distinct deep branches, and you were right about that. Two independent 200k-deep chains in one array ( Objects: agreed, this doesn't cover deeply nested object chains. Destructor ordering: objects still get destroyed immediately via GC: Added |
8d66718 to
a711e95
Compare
|
This also adds significant overhead: From https://github.com/php/php-src/actions/runs/23439349506?pr=21494#summary-68185833867:
|
I Dropped the ds[32] destroy stack. The 256-byte stack allocation on every The buffer wasn't needed. When a second child array hits refcount zero in the same parent, it falls through to What's left is one child pointer (8 bytes), a type check in the cold |
|
@iluuu1994 With latest changes the results are about 2x better But I'll be honest I expected it to be a more... seems like too much overhead still |
30c3cfa to
fb8caa0
Compare
|
Reworked the approach. I added a single Overhead should be one predicted-not-taken comparison per |
|
@iliaal Did you possibly forget to push? I still the the previous implementation. |
65fe737 to
a3469b4
Compare
Actually updating the PR helps, Eh? 🤦That being said I tried the alternative and benchmark actually regressed, going to need to think about this a little more. For now reverting to the patch with best results, but still probably too much of a speed regression to consider |
a3469b4 to
5de093e
Compare
…ted arrays When zend_array_destroy recurses through rc_dtor_func for nested arrays, deeply nested structures (200K+ levels) overflow the C stack. Guard zend_array_destroy with zend_call_stack_overflowed(). When the stack is near its limit, switch to zend_array_destroy_iterative() which uses tail-call elimination for linear chains and a heap-allocated work list for sibling arrays. The hot path is completely unchanged -- only a single stack-limit comparison is added per call.
5de093e to
6399068
Compare
Took another stab at a fix, still not quite ideal, but Symfony impact is now well under 0.1%, sadly Wordpress while faster (3x over original) is still probably in sub-optimal state. Going to leave it here for now...
|
Summary
zend_array_destroy()recurses viai_zval_ptr_dtorfor each element, overflowing the C stack at ~40-50k nesting levels.The fix combines two mechanisms:
Tail-call optimization for the first child array whose refcount reaches zero -- loops back instead of recursing. Zero overhead for linear chains (
$a = [$a]).Destroy stack for additional array children -- when multiple elements are arrays with refcount reaching zero, extras are pushed onto a heap-backed stack (with a small on-stack buffer to avoid allocation in the common case). After the tail-call chain completes, items are popped from the stack and processed iteratively.
This eliminates C stack growth for array destruction regardless of nesting shape: linear chains, COW-shared branches (
$a = [$a, $a]), and independent deep branches all run in constant stack depth.Non-array refcounted values (objects, strings, etc.) are still destroyed immediately via
rc_dtor_func, preserving existing destructor behavior.Fixes #15869