Skip to content
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.

Sign up for GitHub

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

Jump to bottom

BREAKING(ffi/unstable): always return u64 as bigint #23981

Merged
merged 1 commit into from
May 28, 2024
Merged

BREAKING(ffi/unstable): always return u64 as bigint #23981

merged 1 commit into from
May 28, 2024

Conversation

littledivy
Copy link
Member

@littledivy littledivy commented May 25, 2024 •
edited
Loading

The mixed number | bigint representation was useful optimization for pointers. Now, pointers are represented as V8 externals. As part of the FFI stabilization effort we want to make bigint the only representation for u64 and i64.

BigInt representation performance is almost on par with mixed representation with the added benefit that its less confusing and users don't need manual checks and conversions for doing operations on the value.

cpu: AMD Ryzen 5 7530U with Radeon Graphics
runtime: deno 1.43.6+92a8d09 (x86_64-unknown-linux-gnu)

file:///home/divy/gh/ffi/main.ts
benchmark                 time (avg)        iter/s             (min … max)       p75       p99      p995
-------------------------------------------------------------------------- -----------------------------
nop                        4.01 ns/iter 249,533,690.5     (3.97 ns … 10.8 ns) 3.97 ns 4.36 ns 9.03 ns
ret bigint                 7.74 ns/iter 129,127,186.8    (7.72 ns … 10.46 ns) 7.72 ns 8.11 ns 8.82 ns
ret i32                    7.81 ns/iter 128,087,100.5    (7.77 ns … 12.72 ns) 7.78 ns 8.57 ns 9.75 ns
ret bigint (add op)       15.02 ns/iter  66,588,253.2   (14.64 ns … 24.99 ns) 14.76 ns 19.13 ns 19.44 ns
ret i32    (add op)       12.02 ns/iter  83,209,131.8   (11.95 ns … 18.18 ns) 11.98 ns 13.11 ns 14.5 ns

@littledivy littledivy changed the title BREAKING(unstable): always represent FFI u64 as bigint BREAKING(ffi/unstable): always return u64 as bigint May 25, 2024
@littledivy
Copy link
Member Author

Marked as breaking because of the type definition changes. It should not affect runtime usage since we didn't guarantee number | bigint at runtime

@littledivy littledivy requested a review from aapoalas May 25, 2024 05:09
aapoalas
aapoalas approved these changes May 25, 2024
View reviewed changes
Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@aapoalas
Copy link
Collaborator

Actually, I wonder if we don't have some code on the Fast API path that is now irrelevant? I think we might be setting the Integer64Representation to Number, that should now change to BigInt

@littledivy
Copy link
Member Author

Good catch.

Did we ever use kUint64 for u64 return types though? We have some needsUnwrapping logic where its passes a mutable RV as the last value of the fast call:

deno/ext/ffi/dlfcn.rs

Lines 55 to 60 in b21004b

pub fn needs_unwrap(rv: &NativeType) -> bool {
matches!(
rv,
NativeType::I64 | NativeType::ISize | NativeType::U64 | NativeType::USize
)
}

I updated the JS side where the unwrapping happens to always read it as BigInt.

@aapoalas
Copy link
Collaborator

aapoalas commented May 25, 2024 •
edited
Loading

Hmm, didn't I ever get around to supporting them (u64, i64 return values that is) in FFI? 🤔 They're nowadays supported in Fast API though, so we could get rid of the JS-side unwrapping.

@aapoalas aapoalas mentioned this pull request May 25, 2024
Merged
bartlomieju
bartlomieju approved these changes May 27, 2024
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good to see so much code removed.

@littledivy littledivy merged commit 53606de into denoland:main May 28, 2024
17 checks passed
@littledivy littledivy deleted the ffi_u64_bigint branch May 28, 2024 04:01
littledivy pushed a commit that referenced this pull request May 30, 2024
@aapoalas
BREAKING(ffi/unstable): use BigInt representation in turbocall (#23983)
d67ee9a 
Built ontop of #23981, this sets FFI
turbocalls (Fast Call API) to use the BigInt representation.
Milly added a commit to Milly/deno-namedpipe that referenced this pull request Jun 4, 2024
@Milly
fix: changes for FFP API in Deno 1.44
668c8e2 
See: denoland/deno#23981
Milly added a commit to Milly/deno-namedpipe that referenced this pull request Jun 4, 2024
@Milly
fix: changes for FFI API in Deno 1.44
983fd2d 
See: denoland/deno#23981
Milly added a commit to Milly/deno-namedpipe that referenced this pull request Jun 4, 2024
@Milly
fix: changes for FFI API in Deno 1.44
7dd6139 
See: denoland/deno#23981
@Milly Milly mentioned this pull request Jun 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aapoalas aapoalas aapoalas approved these changes

@bartlomieju bartlomieju bartlomieju approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@littledivy @aapoalas @bartlomieju