-
Notifications
You must be signed in to change notification settings - Fork 89
Create solana-sdk-wasm-js crate #138
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
Conversation
a880ac3
to
2e6e1fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main change introduced in this PR is that people need to run wasm-pack
against your new package to get a binary that's usable in JS, and that should be the only breakage.
It's as if solana-sdk
had a binary output and a library output (like solana-gossip), but we're just factoring the binary part into a new crate.
This means we can keep all of the code as is, but just re-export functions from the new crate, along with instructions for building. Does that make sense?
I didn't quite get the idea. Can I still maintain the wasm-specifc code in the new crate, but then re-export it in Solana-program and in solana-sdk? Or is it the opposite? |
Ah, sorry it wasn't clear. The wasm-specific code should still live in the original crate, and the new crate just re-exports everything and can be built into a JS package with |
fbbc3d6
to
99e99ed
Compare
Based on the above, it looks like in order to run LTO, you need the
And yet, later, you are saying that You are saying that you do see a size reduction. So, I'm probably missing something. Or is it that the type of the dependent crates does not matter, and only the type of the final artifact matters? |
@ilya-bobyr The problem is the usage of multiple crate types together. A short answer is that dependencies should be The option The plan is to support LTO via cargo-build-sbf, in which case we need the correct configuration of crate types. Since most solana programs depend on our own library, setting it to rlib facilitates it. |
As per the discussion on Discord (https://discord.com/channels/428295358100013066/476811830145318912/1370445804078235908), I reverted the PR back to its original form. Let me know if there is any other feedback to be addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, and will be an important first step to eventually extracting the wasm-bindgen bits out of the component crates.
program/Cargo.toml
Outdated
@@ -109,10 +109,7 @@ arbitrary = { workspace = true, features = ["derive"] } | |||
solana-logger = { workspace = true } | |||
|
|||
[target.'cfg(target_arch = "wasm32")'.dependencies] | |||
console_error_panic_hook = { workspace = true } | |||
console_log = { workspace = true } | |||
getrandom = { workspace = true, features = ["js", "wasm-bindgen"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed? I thought we'd only need it in the sdk-wasm-js crate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did have a problem before with getrandom
not being compatible with wasm, since it does not implement the random number generation for wasm unless we explicitly pass on the feature. I wasn't able to replicate the issue right now, so I'll remove the import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the dependency in dcab021. Changes got eaten up during rebase.
sdk-wasm-js/Cargo.toml
Outdated
name = "solana-sdk-wasm-js" | ||
description = "Solana SDK Wasm JS" | ||
documentation = "https://docs.rs/solana-sdk-wasm-js" | ||
version = "2.2.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just set this to 1.0.0 to start
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in fa316d5.
sdk/Cargo.toml
Outdated
@@ -197,7 +195,7 @@ all-features = true | |||
rustdoc-args = ["--cfg=docsrs"] | |||
|
|||
[lib] | |||
crate-type = ["cdylib", "rlib"] | |||
crate-type = ["rlib"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
sdk-wasm-js/README.md
Outdated
|
||
More information about Solana is available in the [Solana documentation](https://solana.com/docs). | ||
|
||
The [Solana Program Organization](https://github.com/solana-program) provides examples of how to use this crate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it? I can't think of any places where we use the wasm crate 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was just copy and paste. I removed that in fa316d5.
3a0749d
to
c264065
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Just needs to fix the merge conflict, then we can land this
Thanks! I've just rebased the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks!
Problem
solana-program
andsolana-sdk
have both cdylib and rlib declared as crate types. Both crates are supposed to be used as libraries by other projects. This setting is specially problematic for SBF programs that importsolana-program
, since it does not permit us to run link-time optimizations:The problem lies in using cdylib and rlib together, since that prevents Rust from running LTO, when using Solana-program as a dependency. See rust-lang/rust#51009 for more information.
Summary of changes
solana-sdk-wasm
to hold all the items necessary for such a target.solana-program
andsolana-sdk
.program/src/lib.rs
to correctly mention how to use the crate types.Basic benchmark
This is not a comprehensive benchmark. I was just curious on the impact on a small
Hello, Solana!
program, like this:These are the results:
No LTO: 18kb (18504 bytes) with 728 CUs consumed.
LTO enabled: 17kb (17576 bytes) with 638 CUs consumed.