Draft: --logHeapUsage: also log process id #14410
Conversation
✅ Deploy Preview for jestjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
cben
changed the title
|
I assume the pid would be useless if we used |
|
I think this is good idea! May I suggest little bit different format if this goes forward:
Worker id is only needed in debug sessions, so you are first usually interested about heap size. I also feel, calling it worker would be better that pid but that’s probably just preference. Note, I’m just another jest user and this is not actual review comment from project :) Also, as this leads usually bikeshedding, I’ll leave further discussion for author and maintainers. |
| @@ -94,6 +94,7 @@ export type TestResult = { | |||
| failureMessage?: string | null; | |||
| leaks: boolean; | |||
| memoryUsage?: number; | |||
| workerProcessId?: number; | |||
There was a problem hiding this comment.
I only read this in --logHeapUsage mode, but decided to set it unconditionally — might come useful in some other situations?
However, I wasn't sure it's meaningful in --runInBand mode so didn't know if the type should guarantee it always being there, kept it optional 🤷♂️
This is meant to aid dealing with jestjs#11956 --logHeapUsage after each test logs the heap size of whatever worker happened to run it, but because results are interleaved from multiple workers, one has to guess which heap sizes came from different workers (jest doesn't even log how many workers are involved). => This adds the process id: PASS src/BlaBla.test.ts (9.64 s, pid 1085066: 266 MB heap size) Making it easy to focus on a single worker's heap history, or plot each worker's progression separately: egrep --only-matching 'pid.*heap size' JEST_OUTPUT.txt | sort --field-separator=' ' --key=2 --numeric --stable Printing OS process ids also allows correlating with other perf tools like `top`. It also reflects workers getting shutdown/replaced with `--workerIdleMemoryLimit`: PASS __tests__/test1.js (pid 1104509: 22 MB heap size) PASS __tests__/test2.js (pid 1104519: 22 MB heap size) PASS __tests__/test3.js (pid 1104529: 22 MB heap size)
cben
force-pushed
the
logHeapUsage-process-id
branch
from
efd39c0
to
c3516b6
Compare
|
This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days. |
|
I just found this PR and I think it's extremely valuable - I'm not sure #11956 is actually fixed (I'm still experiencing a similar problem while running latest nodeJS), and in theory even if one leak is fixed, others may occur in the future, either due to bugs in Jest itself, a custom runner / transformer, or actual test files. Adding the worker PID will be very helpful in debugging such leaks, it's a reliable way to judge if the heap size is actually growing or not. Also, if the heap is only growing in one worker, but remains stable in others, it might make it easier to pinpoint problematic test files. |
|
This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days. |
Summary
This is meant to aid dealing with #11956 which remains a pain (workarounds come with some speed cost, so being able to measure on one's project helps decide if they're worth it).
--logHeapUsage after each test logs the heap size of whatever worker happened to run it, but because results are interleaved from multiple workers, one has to guess which heap sizes came from different workers (jest doesn't even log how many workers are involved).
=> This PR adds the process id:
Making it easy to focus on a single worker's heap history, or plot each worker's progression separately:
Using OS process ids also allows correlating with other perf tools like
top.It also reflects workers getting shutdown/replaced with
--workerIdleMemoryLimit: (here had --maxWorkers=2 but one got replaced):Test plan
To be honest, I hadn't run this version of jest yet, just manually applied same changes under node_modules/ of my work project where we're still on jest 26 🤠
mainbranch and passes with these changes, so I guess that counts.I've used the resulting output to plot per-worker histories with and without the memory leak, which immediately taught me new things — I thought ALL fluctuations were interleaving between workers, but it shows per-worker heaps do go down repeatedly:
https://docs.google.com/spreadsheets/d/1vdRXwcBTTxaK65YRoBILyQJHZuEKj_-yyWCidhUotCs/edit#gid=2081209907
WHY DRAFT
TODO: I assume all this won't work with
enableWorkerThreads? Need to logworker.threadId, and to choose output format that accommodates both modes?EDIT: or do all threads share one heap?
update CHANGELOG.md