-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add gc and shutdown callbacks to ZendMM custom handlers
#13432
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
c9186cc to
12da346
Compare
12da346 to
0366f30
Compare
fdf1e73 to
07fe9e1
Compare
07fe9e1 to
3629b00
Compare
gc and shutdown callbacks to ZendMM custom handlers (削除ここまで)gc and shutdown callbacks to ZendMM custom handlers (追記ここまで)
d476664 to
bad957f
Compare
bad957f to
02488dc
Compare
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.
I reviewed only zend_alloc part of the PR, and it looks almost fine.
See the comments about zend_mm_heap_create/zend_mm_heap_free calls.
5df0ef0 to
ee2f0bc
Compare
I did an implementation in https://github.com/realFlowControl/zendmmobserver_custom_handler that shows how to use this to build an observer without relying on tricky things like tinkering with the _zend_mm_heap->use_custom_heap flag which is at risk (see #14570)
Additionally this seems to allow to register in MINIT/MSHUTDOWN and not RINIT/RSHUTDOWN. For correct ZTS support GINIT and GSHUTDOWN should be the way to go though, I will have to play with this a bit.
ee2f0bc to
9d42038
Compare
9d42038 to
06ef9d5
Compare
zend_alloc.* changes look good to me.
@arnaud-lb in observe zendmm I played with it and with this PR, we can (un-)register ZendMM custom handlers in GINIT/GSHUTDOWN already without having any side effects from observing (forwarding back to the original heap). What is also pretty neat is that in case you want to stop observing, you can just call zend_mm_set_heap() with the original heap and if you want to start observing again, just call zend_mm_set_heap() with the observed heap.
I also ran some tests with ext-parallel and FrankenPHP with enabled observe zendmm and everything was stable 🎉
Thank you!
Uh oh!
There was an error while loading. Please reload this page.
Hello everyone 👋
This PR aims to add a
gc()andshutdown()callback to the ZendMM custom heap (currently we only havealloc(),free()andrealloc()). These two new functions would allow to do garbage collection and cleanup in a custom memory manager and/or allow to handle some side effects of using thezend_mm_set_custom_handlers()API when "just" observing the ZendMM (see #11758, "observing" as in passing calls back to ZendMM).The initial idea was to add a
zend_mm_set_custom_handlers_ex()function to only set thegc()andshutdown()hooks, but this has a major drawback: It would look like you could setgcandshutdownhandlers without callingzend_mm_set_custom_handlers()and therefore we'd need to enforce calling the one before the other (which I'd like to refrain from doing) or it would raise complexity in handling cases where one set of handlers exists, while another does not. We could also add ause_custom_heap_exflag, but I'd also like to refrain from doing this, as It would add additional checks in the hot path (during (de-)allocations).I think it makes sense to have the
gcandshutdownbundled with the other hooks. In the current version they are nullable, but I start thinking that they should not.CC: @arnaud-lb