-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Drop zend_mm_set_custom_debug_handlers()
#13457
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
Drop zend_mm_set_custom_debug_handlers()
#13457
Conversation
I think that's reasonable. It makes implementations of custom handlers a bit simpler, especially if they forward the call to another handler.
zend_mm_set_custom_debug_handlers() (削除ここまで)zend_mm_set_custom_debug_handlers() (追記ここまで)
86ead8a to
eeb4e7e
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.
Generally looks good to me, although I have never used the .debug API so that's an important caveat ^_^
This makes calls to user memory handlers more expensive and may require additional redirector to system malloc(). Also, this will probably "break" a few third party PHP extensions.
This change is not critical, but I wouldn't support it.
Thanks for having a look at the PR.
This makes calls to user memory handlers more expensive
In case of ndebug, those ZEND_FILE_LINE_* macros are empty. To my understanding we would save a function call (to _malloc_custom/__free_custom/__realloc_custom) on every single allocation/free/realloc. Only in case of a debug build it would add four arguments to every of those calls, but still save a function call. Am I missing something else?
Also, this will probably "break" a few third party PHP extensions.
You are right, extensions that are using the zend_mm_set_custom_handlers() would require a signature change in case they would still like to be compatible with debug builds (which they currently are). If we are talking about extensions that forward calls back to ZendMM (like the Datadog Profiler or https://github.com/arnaud-lb/php-memory-profiler), those would need top explicitly support debug builds using the ZEND_FILE_LINE_* macros already anyway.
Thanks for having a look at the PR.
This makes calls to user memory handlers more expensive
In case of
ndebug, thoseZEND_FILE_LINE_*macros are empty. To my understanding we would save a function call (to_malloc_custom/__free_custom/__realloc_custom) on every single allocation/free/realloc. Only in case of adebugbuild it would add four arguments to every of those calls, but still save a function call. Am I missing something else?
You are right. My argument was wrong and now I support this.
0f5c255 to
63fe29f
Compare
Uh oh!
There was an error while loading. Please reload this page.
Hey folks,
while working on #13432, I was about to add
gcandshutdownhooks forZEND_DEBUGbuilds and thezend_mm_set_custom_debug_handlers()function. It looked to me that this function serves no specific purpose anymore and we could just remove it in favour of addingZEND_DEBUGsupport to thezend_mm_set_custom_handlers()function. This also saves a function call per everymalloc/realloc/freecall when a custom handler is around (and an additional compare operation in case a custom handler is installed in theZEND_DEBUGcase) and removes some code from ZendMM.A quick GitHub search did not show anyone using this function (it could only be found in forks or other copies from php-src)
This brings one breaking change: extensions that are using the
zend_mm_set_custom_handlers()would require a signature change in case they would still like to be compatible withdebugbuilds (which they currently are). If we are talking about extensions that forward calls back to ZendMM (like the Datadog Profiler or https://github.com/arnaud-lb/php-memory-profiler), those would need to explicitly supportdebugbuilds using the ZEND_FILE_LINE_* macros already anyway (meaning they are notdebugcompatible currently and thus not a bc for them).