Skip to content
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.

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

chore(napi): add oxc-parser to benchmarks #2724

Merged
merged 1 commit into from Mar 15, 2024

Conversation

overlookmotel
Copy link
Collaborator

@overlookmotel overlookmotel commented Mar 15, 2024

Closes #2616.

Adds benchmarks for NodeJS NAPI build. Measurement includes JSON.parse of the AST on JS side, since that's how it'll be used 99% of the time.

Benchmarks run against same files as Rust parser benchmarks, so we can see the overhead of transferring AST to JS.

Copy link
Collaborator Author

overlookmotel commented Mar 15, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @overlookmotel and the rest of your teammates on Graphite Graphite

@overlookmotel overlookmotel force-pushed the 03-15-chore_napi_add_oxc-parser_to_benchmarks branch from 549ec9a to 6e0d135 Compare March 15, 2024 00:50
@overlookmotel
Copy link
Collaborator Author

NB: I initially tried making the JS benchmarks a separate task, but codspeed then only displays 1 set of benchmarks or the other - whichever results arrive latest clobber the other task's results. It seems all benchmarks have to be in a single call to their CodSpeedHQ/action action.

@overlookmotel
Copy link
Collaborator Author

overlookmotel commented Mar 15, 2024

Had to update pnpm as was failing to install with ERR_INVALID_THIS errors.

Seems to be a common problem with pnpm and only apparent solution is to update to latest.

PS: Benchmarks are getting quite slow on CI. Maybe we do need to look into trying to shard them across multiple tasks.

Copy link

codspeed-hq bot commented Mar 15, 2024

CodSpeed Performance Report

Merging #2724 will not alter performance

Comparing 03-15-chore_napi_add_oxc-parser_to_benchmarks (a2ee4ee) with main (53a8e7f)

🎉 Hooray! codspeed-node just leveled up to 3.1.0!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

✅ 29 untouched benchmarks

🆕 5 new benchmarks

Benchmarks breakdown

Benchmark main 03-15-chore_napi_add_oxc-parser_to_benchmarks Change
🆕 parser(napi)[RadixUIAdoptionSection.jsx] N/A 6.3 ms N/A
🆕 parser(napi)[antd.js] N/A 15.3 s N/A
🆕 parser(napi)[cal.com.tsx] N/A 4 s N/A
🆕 parser(napi)[checker.ts] N/A 6.7 s N/A
🆕 parser(napi)[pdf.mjs] N/A 2.5 s N/A

@Boshen
Copy link
Member

Boshen commented Mar 15, 2024

NB: I initially tried making the JS benchmarks a separate task, but codspeed then only displays 1 set of benchmarks or the other - whichever results arrive latest clobber the other task's results. It seems all benchmarks have to be in a single call to their CodSpeedHQ/action action.

Let me call for help.

Edit: https://discord.com/channels/1065233827569598464/1218029341741420635

@overlookmotel
Copy link
Collaborator Author

I think it's basically the same problem as sharding the benchmarks (CodSpeedHQ/action#92). If we can figure out how to do that, we can also move NodeJS/NAPI benchmark into a separate task.

From looking at https://github.com/CodSpeedHQ/runner, I do think this should be possible to do, though it'll require some experiementation.

I'm really keen to get the NAPI benchmarks up and running, so I can get #2457 to a state where it can be merged. Would you consider merging this now, and then returning to the sharding problem later?

@Boshen
Copy link
Member

Boshen commented Mar 15, 2024

Ok let's see what happens.

@Boshen Boshen merged commit af47aeb into main Mar 15, 2024
22 of 37 checks passed
@Boshen Boshen deleted the 03-15-chore_napi_add_oxc-parser_to_benchmarks branch March 15, 2024 11:45
Boshen pushed a commit that referenced this pull request Mar 16, 2024
#2724 added CodSpeed benchmarks for NodeJS `oxc-parser`.

Unfortunately it turns out CodSpeed's results are wildly inaccurate.
Unclear why, but have raised an issue with CodSpeed
(CodSpeedHQ/action#96). In meantime it seems
best to remove the benchmarks as they're not useful at present.
overlookmotel added a commit that referenced this pull request Mar 20, 2024
Add NodeJS parser to benchmarks.

Previous attempt #2724 did not work due CodSpeed producing very
inaccurate results (CodSpeedHQ/action#96).

This version runs the actual benchmarks without CodSpeed's
instrumentation. Then another faux-benchmark runs within Codspeed's
instrumented action and just performs meaningless calculations in a loop
for as long as is required to take same amount of time as the original
uninstrumented benchmarks took.

It's unfortunate that we therefore don't get flame graphs on CodSpeed,
but this seems to be the best we can do for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Benchmark NodeJS parser
2 participants