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.

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

refactor(core): Turn the wasm_streaming_feed binding into ops #11985

Merged

Conversation

andreubotella
Copy link
Contributor

Async WebAssembly compilation was implemented in #11200 by adding two bindings: set_wasm_streaming_callback, which registered a callback to be called whenever a streaming wasm compilation was started, and wasm_streaming_feed, which let the JS callback modify the state of the v8 wasm compiler.

set_wasm_streaming_callback cannot currently be implemented as anything other than a binding, but wasm_streaming_feed does not really need to use anything specific to bindings, and could indeed be implemented as one or more ops. This PR does that, resulting in a simplification of the relevant code.

There are three operations on the state of the v8 wasm compiler that wasm_streaming_feed allowed: feeding new bytes into the compiler, letting it know that there are no more bytes coming from the network, and aborting the compilation. This PR provides op_wasm_streaming_feed to feed new bytes into the compiler, and op_wasm_streaming_abort to abort the compilation. It doesn't provide an op to let v8 know that the response is finished, but closing the resource with Deno.core.close() will achieve that.

Async WebAssembly compilation was implemented in denoland#11200 by adding two
bindings: `set_wasm_streaming_callback`, which registered a callback to
be called whenever a streaming wasm compilation was started, and
`wasm_streaming_feed`, which let the JS callback modify the state of the
v8 wasm compiler.

`set_wasm_streaming_callback` cannot currently be implemented as
anything other than a binding, but `wasm_streaming_feed` does not really
need to use anything specific to bindings, and could indeed be
implemented as one or more ops. This PR does that, resulting in a
simplification of the relevant code.

There are three operations on the state of the v8 wasm compiler that
`wasm_streaming_feed` allowed: feeding new bytes into the compiler,
letting it know that there are no more bytes coming from the network,
and aborting the compilation. This PR provides `op_wasm_streaming_feed`
to feed new bytes into the compiler, and `op_wasm_streaming_abort` to
abort the compilation. It doesn't provide an op to let v8 know that the
response is finished, but closing the resource with `Deno.core.close()`
will achieve that.
@andreubotella
Copy link
Contributor Author

andreubotella commented Sep 11, 2021

The goal of this PR is to simplify the code, rather than to improve performance. But some benchmarks should be done either way.

@andreubotella andreubotella marked this pull request as ready for review September 13, 2021 05:58
core/ops_builtin.rs Outdated Show resolved Hide resolved
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

LGTM

@lucacasonato
Copy link
Member

Thanks @andreubotella!

@lucacasonato lucacasonato merged commit 4d6f412 into denoland:main Sep 13, 2021
@andreubotella andreubotella deleted the wasm-streaming-feed-into-ops branch September 13, 2021 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants