| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
|
I removed the author ready label because I want to run some stress tests to make sure this really fixes things. |
Sorry, something went wrong.
Sorry, something went wrong.
|
Stress test on Windows for this PR: https://ci.nodejs.org/job/node-stress-single-test/212/ Stress test on Raspberry Pi for this PR: https://ci.nodejs.org/view/Stress/job/node-stress-single-test-pi1-fanned/136/ |
Sorry, something went wrong.
|
Stress test show it happening less but still happening. I'm going to move this to draft while I investigate more. |
Sorry, something went wrong.
|
Extending the timeout is the wrong solution. It's not that the test needs more time. It's that there is a race condition so the child process never exits. |
Sorry, something went wrong.
|
@Trott - from my earlier observation (I will soon search and find out where and what it was), I guess it was to do with CPU starvation on systems with very less CPU:
Do you know how many CPUs the failing system has? I think https://github.com/nodejs/node/blob/1de4b3892f55d7883f9c4b0192cf03092a53484f/test/sequential/test-worker-prof.js#L29 is the key. We are reading the whole node executable file and converting that into a single char array that proves to be too slow at times? Should we either read a small part of it, or change the process.execPath to __filename? WDYT? |
Sorry, something went wrong.
|
ok, these are the relevant comments where I had observations on the CPU starvation patterns: |
Sorry, something went wrong.
Let's try changing process.execPath to __filename. (If that doesn't work, I wonder if skipping the test when common.enoughTestCpu is false will work.) |
Sorry, something went wrong.
Sorry, something went wrong.
|
Stress test with this PR: https://ci.nodejs.org/job/node-stress-single-test/215/ |
Sorry, something went wrong.
Test failed on macOS in CI because there were only 8 ticks rather than more than 15. // Test that at least 15 ticks have been recorded for both parent and child
// threads. When not tracking Worker threads, only 1 or 2 ticks would
// have been recorded.
// When running locally on x64 Linux, this number is usually at least 200
// for both threads, so 15 seems like a very safe threshold.
assert(ticks >= 15, `${ticks} >= 15`); |
Sorry, something went wrong.
ok, so this time it was too lightweight to produce enough work. How about increasing the pingpong count from 10 to say 100? given one unit of work is pretty small, increasing the loop iteration should be ok I guess. |
Sorry, something went wrong.
On my local machine, there was no noticeable difference in ticks between n=10 and n=100. But up around n=1000, we start to see a ticks go from around 80 to around 200. So, trying n=1024.... |
Sorry, something went wrong.
|
Stress test: https://ci.nodejs.org/job/node-stress-single-test/216/ ✅ |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Fixes: nodejs#26401 Co-authored-by: Gireesh Punathil <gpunathi@in.ibm.com> PR-URL: nodejs#37372 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Fixes: #26401 Co-authored-by: Gireesh Punathil <gpunathi@in.ibm.com> PR-URL: #37372 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
|
It's probably flaky on node 12.21.0 too: |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Fixes: #26401