Skip to content

fix: update test.sh for Node 21 and up output #305

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: update test.sh for Node 21 and up output #305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
987596f
fix: update test.sh for newer node versions
benminer Feb 6, 2025
88fccc3
fix: add node 21 and 22 to ci workflows
benminer Feb 6, 2025
1e6dc15
Merge branch 'main' into main
rohitpruthi-google Feb 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ jobs:
strategy:
fail-fast: false
matrix:
node: [14, 16, 18, 20]
node: [14, 16, 18, 20, 21, 22]
steps:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
Expand Down
2 changes: 1 addition & 1 deletion system-test/system_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ cd $(dirname $0)
# official releases. https://nodejs.org/en/about/releases/
if [[ -z "$BINARY_HOST" ]]; then
ADDITIONAL_PACKAGES="python3 g++ make"
NODE_VERSIONS=(14 16 18 20)
NODE_VERSIONS=(14 16 18 20 21 22)
else
# Tested versions for pre-built binaries are limited based on
# what node-pre-gyp can specify as its target version.
Expand Down
26 changes: 20 additions & 6 deletions system-test/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,17 +55,31 @@ if [[ "$VERIFY_TIME_LINE_NUMBERS" != "true" ]]; then
npm run compile
fi

NODE_VERSION=$(node -v | cut -d. -f1 | tr -d 'v')
node -v
node --trace-warnings "$BENCHPATH" 10 $VERIFY_TIME_LINE_NUMBERS

if [[ "$VERIFY_TIME_LINE_NUMBERS" == "true" ]]; then
pprof -lines -top -nodecount=2 time.pb.gz | tee $tty | \
grep "busyLoop.*src/busybench.js:[2-3][08-9]"
pprof -filefunctions -top -nodecount=2 heap.pb.gz | tee $tty | \
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last time I looked at this, I did some playing around and found some weird behavior. #284 somehow restores the function name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to dig for a way to force this, it may even require some changes to how pprof is being called altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

# It also emits "anonymous" and "idle" statuses in the output.
# E.G: 1877ms 74.93% 74.93% 1878ms 74.97% (anonymous) file:/tmp/tmp.xyz/busybench/src/busybench.js:34
if [ "$NODE_VERSION" -ge 21 ]; then
grep "anonymous.*busybench.js:3[0-9]" <<< "$output"
else
grep "busyLoop.*src/busybench.js:[23][0-9]" <<< "$output"
fi

heap_output=$(pprof -filefunctions -top -nodecount=2 heap.pb.gz | tee $tty)
grep "busyLoop.*src/busybench.js" <<< "$heap_output"
else
pprof -filefunctions -top -nodecount=2 time.pb.gz | tee $tty | \
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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rohitpruthi-google Not sure how I missed this, but yes, will fix!

else
grep "busyLoop.*src/busybench.ts" <<< "$output"
fi
pprof -filefunctions -top -nodecount=2 heap.pb.gz | tee $tty | \
grep "busyLoop.*src/busybench.ts"
fi
Expand Down