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

Wasm32 support #828

Merged
merged 2 commits into from
Mar 8, 2023
Merged

Wasm32 support #828

merged 2 commits into from
Mar 8, 2023

Conversation

tdelabro
Copy link
Contributor

@tdelabro tdelabro commented Feb 8, 2023

wasm support

Description

This pr introduces wasm support for cairo-rs. Aka a new compilation target.
It also adds the std feature, present by default. And the possibility to remove it to compile a slightly different no_std binary

Copy link
Contributor

@Oppen Oppen left a comment

Choose a reason for hiding this comment

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

Hi @tdelabro. Overall it looks good. I left a few comments and questions tho, let me know what you think.

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Merging #828 (ed77a3f) into main (91bd6b0) will decrease coverage by 0.06%.
The diff coverage is 87.98%.

@@            Coverage Diff             @@
##             main     #828      +/-   ##
==========================================
- Coverage   97.47%   97.42%   -0.06%     
==========================================
  Files          69       69              
  Lines       29049    29120      +71     
==========================================
+ Hits        28315    28369      +54     
- Misses        734      751      +17     
Impacted Files Coverage Δ
cairo-vm-cli/src/main.rs 25.53% <0.00%> (ø)
...t_processor/builtin_hint_processor/blake2s_hash.rs 100.00% <ø> (ø)
..._processor/builtin_hint_processor/blake2s_utils.rs 99.74% <ø> (ø)
...int_processor/builtin_hint_processor_definition.rs 98.69% <ø> (ø)
...uiltin_hint_processor/cairo_keccak/keccak_hints.rs 93.91% <ø> (ø)
...rocessor/builtin_hint_processor/dict_hint_utils.rs 99.23% <ø> (+<0.01%) ⬆️
...t_processor/builtin_hint_processor/dict_manager.rs 96.73% <ø> (ø)
...int_processor/builtin_hint_processor/hint_utils.rs 94.59% <ø> (ø)
...t_processor/builtin_hint_processor/keccak_utils.rs 100.00% <ø> (ø)
...int_processor/builtin_hint_processor/math_utils.rs 97.95% <ø> (-0.01%) ⬇️
... and 64 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tdelabro tdelabro force-pushed the wasm32-support branch 4 times, most recently from b0c18d6 to 6ddeb09 Compare February 19, 2023 17:28
@tdelabro tdelabro force-pushed the wasm32-support branch 4 times, most recently from d56107a to 1188716 Compare February 25, 2023 15:50
Copy link
Contributor

@Oppen Oppen left a comment

Choose a reason for hiding this comment

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

Besides the other comments, an important question. Is the wasm version expected to generate traces or just execute and retrieve the output? If it's the latter, we can keep a sizable chunk of the code as is (specially the parts that would use much more memory) by just avoiding compiling them for nostd.

@Oppen
Copy link
Contributor

Oppen commented Mar 1, 2023

Also, this PR is a breaking change, it require you to add the feature std to your import in order to stay iso functional.
Should we increase the semantic versioning accordingly?

That's up to you. Versions of the form 0.x.y are free to break APIs at any point without bumping minors or majors according to the semver specification. Just don't bump it to 1.x.y.

@tdelabro tdelabro force-pushed the wasm32-support branch 4 times, most recently from cd160db to ebeae18 Compare March 5, 2023 13:49
@tdelabro
Copy link
Contributor Author

tdelabro commented Mar 5, 2023

Maybe core::fmt::Write could be enough? Or simply including conditionally that or the one in std::io::Write depending on whether or not we use std.

I picked core::fmt::Write. Conditionally using one or the other was more annoying than anything because types like Sting implement only the core one, one the io

@tdelabro
Copy link
Contributor Author

tdelabro commented Mar 5, 2023

Besides the other comments, an important question. Is the wasm version expected to generate traces or just execute and retrieve the output? If it's the latter, we can keep a sizable chunk of the code as is (specially the parts that would use much more memory) by just avoiding compiling them for nostd.

I think we do. Imagine running a program in the user browser, generating the trace, sending it to the verifier, and getting the proof back. All in a web app, only running on the user's machine

@tdelabro tdelabro force-pushed the wasm32-support branch 3 times, most recently from c3c8c84 to 18a6641 Compare March 6, 2023 15:21
@tdelabro
Copy link
Contributor Author

tdelabro commented Mar 6, 2023

@Oppen @Jrigada I think it is ready for a new review and maybe a merge

Copy link
Contributor

@Oppen Oppen left a comment

Choose a reason for hiding this comment

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

Last round of changes and it's done.
Please don't use git commit --amend or git rebase. We'll squash it on merge so it doesn't make a difference history-wise and it breaks GitHub making it impossible to see changes from the last review. This means reviewers need to go through all the changes again to find what changed.

Copy link
Contributor

@Oppen Oppen left a comment

Choose a reason for hiding this comment

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

🎉 🎉 🎉

Copy link
Contributor

@fmoletta fmoletta left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!!

@fmoletta fmoletta added this pull request to the merge queue Mar 8, 2023
Merged via the queue into lambdaclass:main with commit c326ba8 Mar 8, 2023
Jrigada added a commit that referenced this pull request Mar 10, 2023
Jrigada added a commit that referenced this pull request Mar 10, 2023
Oppen added a commit that referenced this pull request Mar 10, 2023
Oppen added a commit that referenced this pull request Mar 10, 2023
Oppen added a commit that referenced this pull request Mar 13, 2023
Oppen added a commit that referenced this pull request Mar 14, 2023
* Revert "Revert "Wasm32 support (#828)" (#891)"

This reverts commit 23d941f.

* Some optimizations

* Fix mimalloc build for cairo-vm-cli

* Use 3MiB for execution trace buffer
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