-
Notifications
You must be signed in to change notification settings - Fork 105
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
refactor: simplify async op error construction #1039
refactor: simplify async op error construction #1039
Conversation
const promise = new Promise((resolve, reject) => { | ||
promiseRing[idx] = [resolve, reject]; |
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.
Can you run a op_void_async_deferred
microbenchmark? Slighty worried this will cause a regression because it's in the hot path. Maybe { resolve, reject }
is faster because of shape optimizations.
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.
main:
test bench_op_async_void_deferred ... bench: 739,054 ns/iter (+/- 77,089)
test bench_op_async_void_deferred_nofast ... bench: 715,870 ns/iter (+/- 78,892)
test bench_op_async_void_deferred_return ... bench: 678,101 ns/iter (+/- 38,345)
PR:
test bench_op_async_void_deferred ... bench: 712,190 ns/iter (+/- 61,168)
test bench_op_async_void_deferred_nofast ... bench: 742,969 ns/iter (+/- 52,853)
test bench_op_async_void_deferred_return ... bench: 689,404 ns/iter (+/- 41,140)
PR with suggestion:
test bench_op_async_void_deferred ... bench: 742,152 ns/iter (+/- 66,355)
test bench_op_async_void_deferred_nofast ... bench: 736,991 ns/iter (+/- 70,942)
test bench_op_async_void_deferred_return ... bench: 689,867 ns/iter (+/- 39,801)
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.
If the array allocation is a concern we could also move two arrays of ids instead:
const promiseRing = ArrayPrototypeFill(new Array(RING_SIZE), NO_PROMISE);
const rejectRing = ArrayPrototypeFill(new Array(RING_SIZE), NO_PROMISE);
// later
const promise = new Promise((resolve, reject) => {
promiseRing[idx] = resolve;
promiseReject[idx] = reject;
});
Basically keep two rings at the same time. That should be better for memory and GC pressure.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1039 +/- ##
==========================================
+ Coverage 81.43% 81.66% +0.22%
==========================================
Files 97 102 +5
Lines 23877 27927 +4050
==========================================
+ Hits 19445 22806 +3361
- Misses 4432 5121 +689 ☔ View full report in Codecov by Sentry. |
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
Removes usage of serde_v8 for constructing errors