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

Wasm on the Web #1990

Merged
merged 6 commits into from
Jun 5, 2024
Merged

Wasm on the Web #1990

merged 6 commits into from
Jun 5, 2024

Conversation

Twey
Copy link
Contributor

@Twey Twey commented May 2, 2024

Motivation

We would like to be able to execute application blocks in the browser wallet.

Proposal

Make linera-execution's wasmer feature work on the Web:

  • feature-gate things that need to be different between Linux and JS targets
  • loosen some Send requirements for things that aren't Send on the Web (e.g. wasmer::Instance)
  • replace tokio::task::spawn_blocking with a Web worker (to be done in a future PR).

Test Plan

CI build of linera-core, and manual client worker build in linera-web.

Once the Web wallet is all working as intended, we should add some tests using wasm-bindgen-test.

Release Plan

  • Need to update the developer manual.

Links

@Twey Twey changed the title 04 26 web wasm Wasm on the Web May 2, 2024
@Twey Twey force-pushed the 04-26-Web-Wasm branch 3 times, most recently from 56bb71a to 91b1bb4 Compare May 15, 2024 14:45
@Twey Twey force-pushed the 04-26-Web-Wasm branch from 91b1bb4 to 0a97b7e Compare June 5, 2024 11:29
@Twey
Copy link
Contributor Author

Twey commented Jun 5, 2024

In order to help contributions to linera-web, I'm moving responsibility for point #3 to a new PR so we can get this merged and not have linera-web depend on my fork.

Copy link
Contributor

@ma2bd ma2bd left a comment

Choose a reason for hiding this comment

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

LGTM

use wasmer::NativeEngineExt;

let engine = Engine::headless();
let engine = Engine::default();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe @jvff to confirm this one?

Copy link
Contributor Author

@Twey Twey Jun 5, 2024

Choose a reason for hiding this comment

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

This one is fine, but I'd love @jvff's opinion on the safety of removing canonicalize_nans on the Web.

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed on call, this should be an improbable edge case that doesn't affect consensus, only the possibility of a client proposing a block. We should fix it eventually, but it's not a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's an issue tracking this future work: #2103.

{
let mut compiler_config = wasmer::Singlepass::default();
compiler_config.canonicalize_nans(true);

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove newline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the newline is important here to visually separate the setup from the use of compiler_config.

(Ideally it would be a builder! And then the whole thing would be one expression.)

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO this is all doing one thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's small enough that I'm not particularly bothered by it. I think if we decide to make changes elsewhere I'll include it in the next push — otherwise I'd like to merge it without waiting another hour for CI so I can point linera-web to it 😅

use wasmer::NativeEngineExt;

let engine = Engine::headless();
let engine = Engine::default();
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed on call, this should be an improbable edge case that doesn't affect consensus, only the possibility of a client proposing a block. We should fix it eventually, but it's not a blocker.

@Twey Twey merged commit 11f9838 into linera-io:main Jun 5, 2024
4 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.

3 participants