feat(ops): V8 Fast Calls #15291
Merged
feat(ops): V8 Fast Calls #15291
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
littledivy
changed the title
littledivy
removed
the
ci-test-flaky
bartlomieju
reviewed
Aug 20, 2022
|
I finally managed to understand that the fast bindings are generated to an extra function bound into the original function but... Why? Does the references snapshotting somehow not work with direct binding to the normal op call function? |
aapoalas
reviewed
Aug 21, 2022
| for ctx in ops { | ||
| let ctx_ptr = ctx as *const OpCtx as _; | ||
| references.push(v8::ExternalReference { pointer: ctx_ptr }); | ||
| references.push(v8::ExternalReference { |
There was a problem hiding this comment.
Why are all this external refs now suddenly needed when previously they were not?
aapoalas
reviewed
Aug 21, 2022
| ) { | ||
| for ctx in op_ctxs { | ||
| let ctx_ptr = ctx as *const OpCtx as *const c_void; | ||
| set_func_raw(scope, ops_obj, ctx.decl.name, ctx.decl.v8_fn_ptr, ctx_ptr); | ||
|
|
||
| // If this is a fast op, we don't want it to be in the snapshot. |
There was a problem hiding this comment.
While this explains why the boolean is used here, it does not actually explain why the fast op is not wanted in the snapshot. At least I would like to understand why the fast op is left out: Is it because of the doubling of the OpCtx and slow call function pointer referencing?
aapoalas
reviewed
Aug 21, 2022
| @@ -1344,17 +1344,17 @@ async fn op_flash_next_async( | |||
| // the ContextScope creation is optimized away and the op is as simple as: | |||
| // f(info: *const v8::FunctionCallbackInfo) { let rv = ...; rv.set_uint32(op_flash_next()); } | |||
| #[op] | |||
| fn op_flash_next(op_state: &mut OpState) -> u32 { | |||
| let flash_ctx = op_state.borrow_mut::<FlashContext>(); | |||
| fn op_flash_next(state: &mut OpState) -> u32 { | |||
|
This is awesome - great work @littledivy |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relands auto fast api template generation for ops. This optimization is opt in: