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

JsValue module conversions #2771

Merged
merged 8 commits into from
Nov 5, 2024
Merged

JsValue module conversions #2771

merged 8 commits into from
Nov 5, 2024

Conversation

Twey
Copy link
Contributor

@Twey Twey commented Nov 1, 2024

Motivation

We want to send cached (precompiled) modules between threads. In the JavaScript case, that means we need to be able to convert the modules into a JsValue that is suitable to be sent via postMessage.

Proposal

  • Use a Wasmer patch that exposes conversion to/from JsValue for Wasmer modules (including the type hints and other metadata)
  • Inherit those conversions for our wasm::Module type
  • Pass those through our linera-execution trait objects using a boxed trait object akin to dyn-clone

Test Plan

CI for type-correctness, and the actual conversion behaviour has been tested through linera-web.

Release Plan

  • Nothing to do / These changes follow the usual release cycle.

Links

@Twey Twey force-pushed the js-module-conversions branch 2 times, most recently from 2336392 to 0e1ccad Compare November 1, 2024 13:28
@Twey Twey changed the title Js module conversions JsValue module conversions Nov 1, 2024
@Twey Twey mentioned this pull request Nov 1, 2024
Twey added 2 commits November 1, 2024 13:54
This version includes `JsValue` conversions for `Module` that include
type hints.
@Twey Twey force-pushed the js-module-conversions branch from 0e1ccad to 24410a8 Compare November 1, 2024 13:54
@Twey Twey force-pushed the js-module-conversions branch from 24410a8 to 06e81fc Compare November 1, 2024 15:15
@Twey Twey requested review from jvff and ma2bd November 1, 2024 16:52
linera-execution/src/dyn_convert.rs Outdated Show resolved Hide resolved
linera-execution/src/lib.rs Outdated Show resolved Hide resolved
linera-execution/src/lib.rs Outdated Show resolved Hide resolved
impl<T: Send + Sync> Post for T {}

#[cfg(web)]
const _: () = {
Copy link
Contributor

Choose a reason for hiding this comment

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

A module should just work

Copy link
Contributor Author

@Twey Twey Nov 1, 2024

Choose a reason for hiding this comment

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

A module does also work but it's not quite as nice (have to import super::* and think up a meaningless name). This is a bit of an idiom.

Copy link
Contributor

@ma2bd ma2bd Nov 1, 2024

Choose a reason for hiding this comment

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

Ok I find this syntax a bit sad compared to mod trait_impls or something but if the rest of the team can decide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Vote on this was split so I'm going to take the approval and merge — we can reconsider later :)

linera-execution/src/wasm/mod.rs Outdated Show resolved Hide resolved
linera-execution/src/wasm/mod.rs Outdated Show resolved Hide resolved
@@ -214,6 +213,66 @@ impl UserServiceModule for WasmServiceModule {
}
}

#[cfg(web)]
const _: () = {
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Twey added 5 commits November 4, 2024 15:06
We actually can't remove this `Send` and `Sync` until we can send the
`ExecutionStateRequest`s via `postMessage`, i.e. issue linera-io#2552.  That
issue is far enough divorced from this code that I don't want to link
it here, so let's just remove the TODO.
@Twey Twey merged commit 88743d7 into linera-io:main Nov 5, 2024
5 checks passed
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.

2 participants