-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
perf(ext/node): move winerror binding to rust #17792
Conversation
Are you sure it's that many lines? The diff doesn't show it. What's the size difference of the snapshot? |
@bartlomieju note the JS file got renamed to RS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this PR ready?
# hyperfine --warmup 5 "/tmp/deno-pr17792 run x.js" "/tmp/deno-main run x.js" "deno run x.js"
Benchmark 1: /tmp/deno-pr17792 run x.js
Time (mean ± σ): 23.0 ms ± 0.6 ms [User: 17.1 ms, System: 3.9 ms]
Range (min … max): 22.2 ms … 25.0 ms 112 runs
Benchmark 2: /tmp/deno-main run x.js
Time (mean ± σ): 22.8 ms ± 0.5 ms [User: 18.0 ms, System: 4.1 ms]
Range (min … max): 22.1 ms … 24.5 ms 115 runs
Benchmark 3: deno run x.js (1.30.3)
Time (mean ± σ): 16.3 ms ± 0.5 ms [User: 17.7 ms, System: 4.5 ms]
Range (min … max): 15.6 ms … 18.2 ms 149 runs
It's a bit concerning there's no difference in startup time :/ let's push forward with other modules we listed before fully evaluating it. |
Fully hooked up now, and no difference in execution time :( ...maybe thats because winerror.ts is just constants that don't impact deserilazation as much. IMO Let's continue porting larger parts to Rust. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
16873 lines of JS removed from the snapshot.