feat: report allocation stats in the near-OOM profile - #410
IlyasShabi wants to merge 2 commits into
Conversation
Overall package sizeSelf size: 2.63 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | pprof-format | 2.3.1 | 504.33 kB | 504.33 kB | | source-map | 0.8.0 | 185.66 kB | 185.66 kB | | node-gyp-build | 4.8.4 | 13.86 kB | 13.86 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 2 Pipeline jobs failed
Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 1969e5d | Docs | View more details | Give us feedback! |
c6bdb4a to
aa9604c
Compare
aa9604c to
1c8c7e5
Compare
1c8c7e5 to
c682c6e
Compare
There was a problem hiding this comment.
See the remark on fd/file leaks below.
I also had a clanker look at it and it produced these remarks:
Older issues the PR didn't introduce
- dumpAllocationProfileAsJSON doesn't escape names. On Windows, backslashes in script paths would make the new JSON.parse in check_profile.ts fail. There's no Windows CI, so nothing would catch it.
- In allocation mode, profile() stops and restarts the profiler, which also drops the out-of-memory monitoring settings.
| } | ||
| FILE* file = fdopen(fd, "w"); | ||
| if (!file) { | ||
| fprintf(stderr, "Failed to open temp file: %s\n", strerror(errno)); |
There was a problem hiding this comment.
Since you can't call fclose here, you'll leak fd and leave a temp file around. I think you should call close(fd) and delete the temp file.
If you want to be comprehensive, the other "if" statements below have the same problem (and had them before your change :-).) The one on line 231 should also delete the temp file. Then you have the ifs for uv_timer_init and uv_timer_start – I'd write it so that failure in either init or start doesn't return, but falls through to uv_run and unlink (obviously, don't call start if init failed.) I think that'd make sure neither FD is leaked nor a temp file is left around. Since we're in a low memory situation, any of these can run into an ENOMEM.
This branch has not been deployed
2 participants
What does this PR do?:
Motivation:
Additional Notes:
How to test the change?: