Adjust GC threshold if num_roots higher than threshold after collection #13758
Conversation
Zend/tests/gh13670_001.phpt
Outdated
| } | ||
| } | ||
|
|
||
| for ($i = 0; $i < 10001; $i++) { |
There was a problem hiding this comment.
Can these magical numbers be defined using shared consts for all GC tests?
There was a problem hiding this comment.
Good point. I've updated the test to use gc_status()['threshold'] to fetch the default threshold.
This fixes an edge causing the GC to be triggered repeatedly. Destructors might add potential garbage to the buffer, so it may happen that num_root it higher than threshold after collection, thus triggering a GC run almost immediately. This can happen by touching enough objects in a destructor, e.g. by iterating over an array. If the happens again in the new run, and the threshold is not updated, the GC may be triggered repeatedly. Also prevent GC runs during zend_gc_check_root_tmpvars(), as the threshold is not adjusted yet a this point.
|
|
||
| /* Prevent GC from running during zend_gc_check_root_tmpvars, before | ||
| * gc_threshold is adjusted, as this may result in unbounded recursion */ | ||
| GC_G(gc_active) = 1; | ||
| zend_gc_check_root_tmpvars(); |
There was a problem hiding this comment.
Will this lead to store after gc_grow_root_buffer() instead of immediate collection?
I didn't understand, how this is related to the main part of the patch and why this might be a problem.
Please explain.
There was a problem hiding this comment.
Will this lead to store after gc_grow_root_buffer() instead of immediate collection?
Yes exactly
I didn't understand, how this is related to the main part of the patch and why this might be a problem
The main part of the patch increases gc_threshold when num_roots is still >= gc_threshold after collection, to prevent a collection from being triggered immediately. However, this adjustment happens after zend_gc_check_root_tmpvars(), so this function may still trigger an immediate collection. Moreover this happens recursively and it leads to a stack overflow in one of the test cases.
By setting gc_active=1 before zend_gc_check_root_tmpvars(), we prevent collections temporarily.
An alternative would be to adjust the threshold before zend_gc_check_root_tmpvars(). This requires a few more changes because the threshold should not be adjusted when collection is triggered manually, for example.
* PHP-8.2: [ci skip] Adjust GC threshold if num_roots is higher than gc_threshold after collection (#13758)
* PHP-8.3: [ci skip] [ci skip] Adjust GC threshold if num_roots is higher than gc_threshold after collection (#13758)
This fixes an edge case causing the GC to be triggered repeatedly.
Destructors might add potential garbage to the buffer, so it may happen that num_root it higher than threshold after collection, thus triggering a GC run almost immediately. This can happen by touching enough objects in a destructor, e.g. by iterating over an array. If this happens again in the new run, and the threshold is not updated, the GC may be triggered again.
The edge case requires specific conditions to be triggered and it must happen rarely in practice:
The fix is to increase the threshold if
GC_G(num_roots) >= GC_G(gc_threshold)after GC. The threshold eventually reaches a point at which the second condition is not met anymore.The included tests trigger more than 200 GC runs before the fix, and 2 after the fix (dtors always trigger a second run).
A related issue is that zend_gc_check_root_tmpvars() may add potential garbage before the threshold is adjusted, which may trigger GC and exhaust the stack. This is fixed by setting GC_G(active)=1 around zend_gc_check_root_tmpvars().
Fixes #13670