-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Mark arrays as collectable at runtime #9979
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is interesting, but I afraid the additional checks may make slowdown instead of expected speedup. This should be carefully benchmarked.
Arrays are now marked with GC_NOT_COLLECTABLE by default. Only objects and references can actually contribute to a cycle. Once an object or reference is added to the array the GC_NOT_COLLECTABLE flag is removed. This patch does never re-add the flag when the potentially cyclic element is removed. This would be possible but would require tracking the number of potentially elements on override/remove.
39440bc
to
fb5a955
Compare
I tried benchmarking with Valgrind as well as with Symfony Demo /
Symfony Demo /de/blog/:
PHPStan
|
static zend_always_inline bool gc_ht_check_collectable(HashTable *ht) { | ||
return (GC_FLAGS(ht) & (GC_PERSISTENT|GC_NOT_COLLECTABLE)) == GC_NOT_COLLECTABLE; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the function is confusing. It doesn't tell what kind of check we do (positive or negative). I would propose to change it into gc_ht_is_not_collectable()
or may be better into gc_ht_is_collectable()
with corresponding condition changes.
It looks like the current implementation doesn't make significant effect. Some apps become a bit faster others are slower. I think we should postpone this and think about non-collectable objects first. After disabling creation of dynamic object properties (they are deprecated in PHP-8.2) we may determine non-collectable classes just analysing their properties. Then it would make sense to adopt Also the GC algorithm might be improved. See the related ideas described at http://www.eng.usf.edu/~chang5/papers/12/RC_chang_12.pdf |
@dstogov Thank you for taking the time to review this. That works for me, let's postpone this and see if it works better in combination with non-collectable objects, which won't require flagging at runtime. |
Arrays in this PR are marked with GC_NOT_COLLECTABLE by default. In arrays, only objects and references can actually contribute to a cycle because cycles with just arrays are impossible to create from userland. Once an object or reference is added to the array the GC_NOT_COLLECTABLE flag is removed. This patch does not re-add the flag when the potentially cyclic element is removed. This would be possible but would require tracking the number of elements on override/remove.
Benchmarks are yet to be done. This touches several hot-paths so we'll need to make sure it's worth it. JIT also needs an adjustment in one of the handlers which I'll look into. @dstogov I'd be very glad to hear your general thoughts before continuing to avoid working on something potentially non-feasible.
Btw, something similar might be possible for objects once dynamic properties gets fully removed.
When:
then the object of the given class doesn't need to be added to the gc buffer. The two could also be combined, meaning non-cyclic classes added to arrays can't make the arrays cyclic.