Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

perf(ext/ffi): optimize synchronous calls #14945

Merged
merged 10 commits into from
Jun 29, 2022

Conversation

littledivy
Copy link
Member

This commit implements an optimization
for blocking FFI calls.

op_ffi_load creates JS functions for
synchronous symbols. This avoids
reallocations that existed in op_ffi_call
and allows future V8 fast API optimizations.

Each function owns an allocation to corresponding
the Symbol, which is freed on GC.

Nonblocking ops & statics are still registered JS
side and use ops to make calls.

image

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, these are very impressive results

@lucacasonato
Copy link
Member

I want to discuss this in CLI design meeting, because it bypasses ops.

@lucacasonato
Copy link
Member

Also unintentional SWC downgrade?

@aapoalas
Copy link
Collaborator

Taking Fast API into usage will require using FunctionTemplate which means that the function's lifetime becomes tied to the isolate, ie. un-GC'able.

This doesn't mean it shouldn't necessarily be done but probably does mean that it should be opt-out'able.

ext/ffi/00_ffi.js Outdated Show resolved Hide resolved
ext/ffi/lib.rs Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member

Also unintentional SWC downgrade?

How so?

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.

@lucacasonato
Copy link
Member

How so?

Look at cli/Cargo.toml

@bartlomieju
Copy link
Member

How so?

Look at cli/Cargo.toml

Screenshot 2022-06-23 at 12 42 49

I see only two changed files

@lucacasonato
Copy link
Member

Huh, on the GitHub mobile app I see a bunch more changed files. I guess must be a bug. It looks correct on the website... sorry for the noise.

@littledivy littledivy merged commit 76d387f into denoland:main Jun 29, 2022
dsherret pushed a commit to dsherret/deno that referenced this pull request Jun 30, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants