-
Notifications
You must be signed in to change notification settings - Fork 78
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
Implement auto dump on out-of-memory #123
base: master
Are you sure you want to change the base?
Conversation
Two new INI settings have been added called `meminfo.dump_on_limit` and `meminfo.dump_dir`. When dump_on_limit is enabled, meminfo will attempt to create a heap dump when an OOM error is detected in the `dump_dir` directory. OOM errors are detected by registering an error handler, then checking for a prefix of "Allowed memory size of" on the error message.
@BitOne When I did my original implementation, I did some testing with a real world scenario, but I haven't done that yet with this implementation. I did test some trivial examples (and added a test for it). My main concern is if we'll get a secondary OOM error when we try to dump a very large "real world" heap. Hopefully changing the memory limit before we try the dump will handle this though. 🤞 |
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.
Can you check the indentation to make it homogeneous with the rest of the code?
Otherwise, pretty cool, stuff thank you!
@@ -552,6 +620,23 @@ zend_string * meminfo_escape_for_json(const char *s) | |||
return s3; | |||
} | |||
|
|||
#define MEMORY_LIMIT_ERROR_PREFIX "Allowed memory size of" |
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.
Is there no other way to check for memory exhaustion error?
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 found this technique in memprof when I was researching how to do it. Unfortunately I didn't find any other options.
|
||
PHP_INI_BEGIN() | ||
STD_PHP_INI_ENTRY("meminfo.dump_on_limit", "Off", PHP_INI_ALL, OnUpdateBool, dump_on_limit, zend_meminfo_globals, meminfo_globals) | ||
PHP_INI_ENTRY("meminfo.dump_dir", "/tmp", PHP_INI_ALL, NULL) |
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.
@BitOne Do you think we can just let this default to "/tmp" for now? It might not be the best for Windows, but I think most users will probably set the dump_dir if they are going to use this anyway right?
Two new INI settings have been added called
meminfo.dump_on_limit
andmeminfo.dump_dir
. When dump_on_limit is enabled, meminfo will attemptto create a heap dump when an OOM error is detected in the
dump_dir
directory.
OOM errors are detected by registering an error handler, then checking
for a prefix of "Allowed memory size of" on the error message.
dump_dir
on windows?resolves #122