Skip to content

Add gc and shutdown callbacks to ZendMM custom handlers - #13432

Merged
arnaud-lb merged 4 commits into
php:masterfrom
realFlowControl:zendmm-custom-handlers
Jun 19, 2024
Merged

Add gc and shutdown callbacks to ZendMM custom handlers#13432
arnaud-lb merged 4 commits into
php:masterfrom
realFlowControl:zendmm-custom-handlers

Conversation

@realFlowControl

@realFlowControlrealFlowControl commented Feb 19, 2024

Copy link
Copy Markdown
Contributor

Hello everyone 👋

This PR aims to add a gc() and shutdown() callback to the ZendMM custom heap (currently we only have alloc(), free() and realloc()). 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 the zend_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 the gc() and shutdown() hooks, but this has a major drawback: It would look like you could set gc and shutdown handlers without calling zend_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 a use_custom_heap_ex flag, 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 gc and shutdown bundled with the other hooks. In the current version they are nullable, but I start thinking that they should not.

CC: @arnaud-lb

@realFlowControl
realFlowControlforce-pushed the zendmm-custom-handlers branch 2 times, most recently from c9186cc to 12da346CompareFebruary 19, 2024 10:34
@realFlowControl
realFlowControlforce-pushed the zendmm-custom-handlers branch 8 times, most recently from fdf1e73 to 07fe9e1CompareFebruary 21, 2024 14:21
@realFlowControlrealFlowControl changed the title [WIP] Add gc and shutdown callbacks to ZendMM custom handlersAdd gc and shutdown callbacks to ZendMM custom handlersFeb 26, 2024
@realFlowControl
realFlowControlforce-pushed the zendmm-custom-handlers branch 3 times, most recently from d476664 to bad957fCompareFebruary 29, 2024 09:09
@realFlowControl
realFlowControl marked this pull request as ready for review June 4, 2024 11:46

@dstogovdstogov left a comment

Copy link
Copy Markdown
Member

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.

Comment threadZend/zend_alloc.c Outdated
Comment threadZend/zend_alloc.c Outdated
Comment threadZend/zend_alloc.c Outdated
@realFlowControl
realFlowControlforce-pushed the zendmm-custom-handlers branch 2 times, most recently from 5df0ef0 to ee2f0bcCompareJune 17, 2024 07:14
@realFlowControl

Copy link
Copy Markdown
ContributorAuthor

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.

@dstogov

Copy link
Copy Markdown
Member

zend_alloc.* changes look good to me.

@realFlowControl

realFlowControl commented Jun 19, 2024

Copy link
Copy Markdown
ContributorAuthor

@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 🎉

@arnaud-lb
arnaud-lb merged commit f4557b4 into php:masterJun 19, 2024
@realFlowControl
realFlowControl deleted the zendmm-custom-handlers branch June 19, 2024 17:45
arnaud-lb added a commit that referenced this pull request Jun 19, 2024
@arnaud-lb

Copy link
Copy Markdown
Member

Thank you!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@realFlowControl@dstogov@arnaud-lb