fix: update test.sh for Node 21 and up output
#305
Conversation
|
|
| grep "busyLoop.*src/busybench.js" | ||
| output=$(pprof -lines -top -nodecount=2 time.pb.gz | tee $tty) | ||
|
|
||
| # Due to V8 changes in Node 21, the line numbers are different. |
There was a problem hiding this comment.
Thanks for the PR. It's unfortunate that the function name is lost. Do you have any more info on this V8 change or if there is something we can do to workaround it?
It's good to see the output is the same in older node versions.
There was a problem hiding this comment.
Last time I looked at this, I did some playing around and found some weird behavior. #284 somehow restores the function name
There was a problem hiding this comment.
Ah, hm. I am no v8 expert, but from some quick searches, this could be because v8's optimizer was changed/improved in version 11 (which node21 is using). Since busyLoop is in the "hot path", there might be some under the hood optimizations happening to it that hide it from the profiler, which adding noop to break it up removes.
There was a problem hiding this comment.
I'll try to dig for a way to force this, it may even require some changes to how pprof is being called altogether.
There was a problem hiding this comment.
I think I just proved this too, adding this native syntax yields function name in v21 (without the noop function)
function busyLoop(durationSeconds) {
%NeverOptimizeFunction(busyLoop);
You have to execute the benchmark with --allow-natives-syntax, though.
The output:
Showing top 2 nodes out of 6
flat flat% sum% cum cum%
1805ms 68.58% 68.58% 2606ms 99.01% (anonymous) file:/tmp/tmp.YTvrn84YUd/busybench/src/busybench.js:37
634ms 24.09% 92.67% 634ms 24.09% busyLoop file:/tmp/tmp.YTvrn84YUd/busybench/src/busybench.js:32'
interesting that there is still one trace that is anonymous, though,
There was a problem hiding this comment.
@benminer that makes sense and kind of what I figured. Thank you for looking into this. It sounds good to merge this then, I'll let @rohitpruthi-google take it over.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #305 +/- ##
=======================================
Coverage 41.92% 41.92%
=======================================
Files 14 14
Lines 2092 2092
Branches 42 42
=======================================
Hits 877 877
Misses 1197 1197
Partials 18 18 ☔ View full report in Codecov by Sentry. |
system-test/test.sh
Outdated
| grep "busyLoop.*src/busybench.ts" | ||
| output=$(pprof -filefunctions -top -nodecount=2 time.pb.gz | tee $tty) | ||
| if [ "$NODE_VERSION" -ge 21 ]; then | ||
| grep "anonymous.*busybench.ts" <<< "$output" |
There was a problem hiding this comment.
The tests are failing for node 21 because of the error:
++ pprof -filefunctions -top -nodecount=2 time.pb.gz
++ tee
Main binary filename not available.
+ output='Type: wall
Time: Feb 18, 2025 at 12:04pm (UTC)
Duration: 10.04s, Total samples = 9.26s (92.25%)
Showing nodes accounting for 8.74s, 94.40% of 9.26s total
Dropped 53 nodes (cum <= 0.05s)
Showing top 2 nodes out of 6
flat flat% sum% cum cum%
7.51s 81.13% 81.13% 7.52s 81.24% (anonymous) /tmp/tmp.wxlgZ4mMRa/busybench/build/src/busybench.js
1.23s 13.28% 94.40% 1.23s 13.28% (idle)'
+ '[' 21 -ge 21 ']'
+ grep 'anonymous.*busybench.ts'
** TEST FAILED **
Could you please change this to busybench.js here? I am assuming the source reported here is from build files, hence the different extension.
There was a problem hiding this comment.
@rohitpruthi-google Not sure how I missed this, but yes, will fix!
There was a problem hiding this comment.
Any ETA when it will be merged/released?
There was a problem hiding this comment.
@acierto I've just fixed all of the tests, it's up to @rohitpruthi-google to review now!
|
@rohitpruthi-google I believe I've addressed the feedback, and all tests are passing for me on my machine! |
|
Hey @benminer , is it planned to merge sometime soon? |
hey @AmitNimrodiBringg - I am not in control of this repo, so I am not sure when they will get around to it! I might raise it with my Google contacts because it's been sitting for months now :) To get around your Node22 issues, I recommend overriding pprof with this fork, like so: Hopefully, this can be temporary until the pprof team can take a look at this again. |
|
@aabmass @rohitpruthi-google hey guys - just bumping this, would love to get this merged after 6 months. |
|
hey @benminer @aabmass @rohitpruthi-google pinging again ☝️ Would really appreciate your reply |
8 participants
No description provided.