-
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
feat(ext/ffi): Callbacks #14663
Merged
littledivy
merged 72 commits into
denoland:main
from
aapoalas:feat/ffi-register-callback-rewrite
Jun 20, 2022
Merged
feat(ext/ffi): Callbacks #14663
Changes from 60 commits
Commits
Show all changes
72 commits
Select commit
Hold shift + click to select a range
291ca64
Start rewrite
aapoalas 6f955d0
Further work
aapoalas 352d98e
Scope in stack or not
littledivy 15261ea
Functioning synchronous registered callbacks
aapoalas f98424f
Don't borrow across callback call
littledivy 8b46001
Move forward with parameter types in FFI callbacks
aapoalas 7a10c38
Adapt, does not function properly
aapoalas 2335d2a
Use serde_v8::Value
aapoalas f63ceba
fmt
aapoalas cd8260d
Pick ops stuff from sqlite bindings
aapoalas 78a12cd
Try async fixing
aapoalas 4de8ef8
Work work
aapoalas 8a6dace
Somewhat handle nonblocking calls
aapoalas 90c932d
Not really well working
aapoalas 3c10894
Fix lifetime issues with ffi_parse_args
aapoalas e3b55f6
Error handling
aapoalas 8e63797
async work, thanks divy
aapoalas 15d0fbf
Return lifetimes
aapoalas 69ae9bf
fmt & lint
aapoalas 48f439d
fmt & lint
aapoalas 23f5800
Actually pass RegisteredCallback RID to FFI
aapoalas 6971d16
Actually functioning registered callbacks!
aapoalas e271cc7
fmt & lint
aapoalas 98110af
Remove likely unnecessary active scope tracking from ffi calls
aapoalas ccdd122
ops: support a result returning a future returning a result.
littledivy 283dc15
reenable nonblocking ops as hybrids
littledivy 579015c
Merge branch 'main' into feat/ffi-register-callback-rewrite
aapoalas 4fd175f
Rewrite ext/ffi to return serde_v8::Value
aapoalas f4a8fe2
Fix
aapoalas f1eed63
Move parameter validity checks to JS side
aapoalas 7478ead
Fix nonblocking calls by reverting them to json::Value and U32x2 retu…
aapoalas c98241b
Add extensive FFI baseline benchmarks
aapoalas 6c0ffe8
fmt & lint
aapoalas 297dab7
Minor improvements
aapoalas 87e10ca
Fix tests FFI calls
aapoalas f357cea
Merge branch 'main' into feat/ffi-register-callback-rewrite
aapoalas 7b2c176
fmt
aapoalas 3d53899
Fix more FFI ops call signatures
aapoalas 2f370c9
Fix check_unstable2 usage
aapoalas e6fad7a
Fix mistaken Symbol usage with ptr FFI calls
aapoalas 06dee1f
Fix buffer returning with quite manual V8 value picking
aapoalas c29d8fd
Add callback tests
aapoalas 69eec01
fmt & lint
aapoalas 6db458b
Expose registered callback pointer values
aapoalas 84ddbda
Use primordials
aapoalas bcc5caf
Improve deno_ffi_callback inline comment
aapoalas 77f3f51
Catch and rethrow errors from FFI callbacks
aapoalas ce5b3cb
Fix idiosyncracies, add some types at least
aapoalas 3f11770
Use pointer values for RegisteredCallbacks, removing resource_table u…
aapoalas 53bfa8f
Fix errors in i64 / u64 callback return values
aapoalas 7d75f7d
Review comments
aapoalas 3b8a881
Rename RegisteredCallback to UnsafeCallback
aapoalas 6ac3471
Use specific APIs for ints
aapoalas a781463
Use specific APIs for ints in callbacks, avoid serde_v8 in callback p…
aapoalas 0c67852
Implicit drop
aapoalas d936617
Review fixes, light renaming
aapoalas 798ebf5
Safety notice on UnsafeCallbackResource close()
aapoalas 56004f2
Missing permissions check, remove left-over op, add notice about Unsa…
aapoalas 3cf98c4
Return v8::Value directly in UnsafeCallback constructor
aapoalas 3582d94
Merge branch 'main' into feat/ffi-register-callback-rewrite
littledivy 179074d
Merge branch 'main' into feat/ffi-register-callback-rewrite
littledivy 9f5c8df
Fix UnsafeCallback types and add extensive type tests
aapoalas a84c3f3
Merge branch 'main' into feat/ffi-register-callback-rewrite
aapoalas fc2f2e1
Fix format
aapoalas 27accd9
Remove unnecessary try-catch from deno_ffi_callback
aapoalas 4131bfa
Undo unrelated formatting changes
piscisaureus 2eeffd0
Merge branch 'main' into feat/ffi-register-callback-rewrite
littledivy bd4c1f7
Merge branch 'main' of github.com:denoland/deno into feat/ffi-registe…
littledivy 5184ddd
Add test for error propogation
littledivy 40aac23
Format
littledivy 0780dbc
throwCallback.close()
littledivy 3218386
Merge branch 'main' into feat/ffi-register-callback-rewrite
littledivy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
Deno.core.opSync("op_ffi_read_i16", [0, 0]); | ||
Deno.core.opSync("op_ffi_read_i16", 0n); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
Deno.core.opSync("op_ffi_read_u32", [0, 0]); | ||
Deno.core.opSync("op_ffi_read_u32", 0n); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
Deno.core.opSync("op_ffi_read_i32", [0, 0]); | ||
Deno.core.opSync("op_ffi_read_i32", 0n); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
Deno.core.opSync("op_ffi_read_u64", [0, 0]); | ||
Deno.core.opSync("op_ffi_read_u64", 0n); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
Deno.core.opSync("op_ffi_read_f32", [0, 0]); | ||
Deno.core.opSync("op_ffi_read_f32", 0n); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
Deno.core.opSync("op_ffi_read_f64", [0, 0]); | ||
Deno.core.opSync("op_ffi_read_f64", 0n); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,5 @@ | ||
Deno.core.opSync("op_ffi_call_ptr", { | ||
pointer: [0, 0], | ||
def: { | ||
name: null, | ||
parameters: [], | ||
result: "void", | ||
}, | ||
Deno.core.opSync("op_ffi_call_ptr", 0n, { | ||
name: null, | ||
parameters: [], | ||
buffers: [], | ||
}); | ||
result: "void", | ||
}, []); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,5 @@ | ||
Deno.core.opAsync("op_ffi_call_ptr_nonblocking", { | ||
pointer: [0, 0], | ||
def: { | ||
name: null, | ||
parameters: [], | ||
result: "void", | ||
}, | ||
Deno.core.opAsync("op_ffi_call_ptr_nonblocking", 0n, { | ||
name: null, | ||
parameters: [], | ||
buffers: [], | ||
}); | ||
result: "void", | ||
}, []); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
Deno.core.opSync("op_ffi_buf_copy_into", [[0, 0], new Uint8Array(0), 0]); | ||
Deno.core.opSync("op_ffi_buf_copy_into", 0n, new Uint8Array(0), 0); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
Deno.core.opSync("op_ffi_cstr_read", [0, 0]); | ||
Deno.core.opSync("op_ffi_cstr_read", 0n); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
Deno.core.opSync("op_ffi_read_u8", [0, 0]); | ||
Deno.core.opSync("op_ffi_read_u8", 0n); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
Deno.core.opSync("op_ffi_read_i8", [0, 0]); | ||
Deno.core.opSync("op_ffi_read_i8", 0n); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
Deno.core.opSync("op_ffi_read_u16", [0, 0]); | ||
Deno.core.opSync("op_ffi_read_u16", 0n); |
Oops, something went wrong.
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.
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.
I was thinking, maybe it makes things more explicit if we had
NativeParameterType
andNativeReturnType
, as in:NativeParameterType
can be sent from JS to CNativeReturnType
can be sent from C to JSOf course in case of callbacks the types would be reversed: parameters would be
NativeReturnType
and the return value would have typeNativeParameterType
.So maybe we should consider
ToNativeType
andFromNativeType
or something.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.
I agree, and do plan on doing a rework on the FFI TS types. f.ex. Currently
NativeType
is the "base type" andNativeNumberType
is a narrowed down type from that. I will do a follow-up to rework the types into something that hopefully resembles a reasonable whole.A
ToNativeType
andFromNativeType
sounds splendid for that.