-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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(ops): Monomorphic sync op calls #15337
perf(ops): Monomorphic sync op calls #15337
Conversation
If it makes it easier to get a first iteration done here, IMO it's reasonable to just do a swift inlining pass that ignores eluding |
Yeah, I think a lot of the files I'll just be doing that. |
This reverts commit df687e5.
So, apparently this is ready for review and merge if nothing out of order is found. The tests failed once in parsing strace output but the failure did not repeat on another go around, so apparently it was a flake. |
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.
love it!
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, thank you @aapoalas, great work
Welcome to better optimised op calls! Currently opSync is called with parameters of every type and count. This most definitely makes the call megamorphic. Additionally, it seems that spread params leads to V8 not being able to optimise the calls quite as well (apparently Fast Calls cannot be used with spread params). Monomorphising op calls should lead to some improved performance. Now that unwrapping of sync ops results is done on Rust side, this is pretty simple: ``` opSync("op_foo", param1, param2); // -> turns to ops.op_foo(param1, param2); ``` This means sync op calls are now just directly calling the native binding function. When V8 Fast API Calls are enabled, this will enable those to be called on the optimised path. Monomorphising async ops likely requires using callbacks and is left as an exercise to the reader.
Welcome to better optimised op calls! Currently
opSync
is called with parameters of every type and count. This most definitely makes the call megamorphic. Additionally, it seems that spread params leads to V8 not being able to optimise the calls quite as well (apparently Fast Calls cannot be used with spread params).Monomorphising op calls should lead to some improved performance. Now that unwrapping of sync ops results is done on Rust side, this is pretty simple:
This means sync op calls are now just directly calling the native binding function. When V8 Fast API Calls are enabled, this will enable those to be called on the optimised path.
Monomorphising async ops likely requires using callbacks and is left as an exercise to the reader.
Performance
Comparison of
nop
op in a vacuum:Comparison of FFI related ops:
Discussion
There are good things about this:
Downsides are: