Skip to content
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

Add gc and shutdown callbacks to ZendMM custom handlers #13432

Merged
merged 4 commits into from
Jun 19, 2024

Conversation

realFlowControl
Copy link
Contributor

@realFlowControl realFlowControl commented Feb 19, 2024

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 realFlowControl force-pushed the zendmm-custom-handlers branch 2 times, most recently from c9186cc to 12da346 Compare February 19, 2024 10:34
@realFlowControl realFlowControl force-pushed the zendmm-custom-handlers branch 8 times, most recently from fdf1e73 to 07fe9e1 Compare February 21, 2024 14:21
@realFlowControl realFlowControl changed the title [WIP] Add gc and shutdown callbacks to ZendMM custom handlers Add gc and shutdown callbacks to ZendMM custom handlers Feb 26, 2024
@realFlowControl realFlowControl force-pushed the zendmm-custom-handlers branch 3 times, most recently from d476664 to bad957f Compare February 29, 2024 09:09
@realFlowControl realFlowControl marked this pull request as ready for review June 4, 2024 11:46
Copy link
Member

@dstogov dstogov left a 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.

Zend/zend_alloc.c Outdated Show resolved Hide resolved
Zend/zend_alloc.c Outdated Show resolved Hide resolved
Zend/zend_alloc.c Outdated Show resolved Hide resolved
@realFlowControl realFlowControl force-pushed the zendmm-custom-handlers branch 2 times, most recently from 5df0ef0 to ee2f0bc Compare June 17, 2024 07:14
@realFlowControl
Copy link
Contributor Author

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
Member

dstogov commented Jun 18, 2024

zend_alloc.* changes look good to me.

@realFlowControl
Copy link
Contributor Author

realFlowControl commented Jun 19, 2024

@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:master Jun 19, 2024
10 of 11 checks passed
@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
Member

Thank you!

Sign up for free to 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.

None yet

3 participants