-
Notifications
You must be signed in to change notification settings - Fork 119
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
Use -Clinker-plugin-lto
flag during Wasm build
#358
Use -Clinker-plugin-lto
flag during Wasm build
#358
Conversation
From: Original wasm size: 68.6K, Optimized: 31.9K To: Original wasm size: 67.2K, Optimized: 31.0K
src/cmd/build.rs
Outdated
@@ -249,7 +249,7 @@ fn exec_cargo_for_wasm_target( | |||
// Currently will override user defined RUSTFLAGS from .cargo/config. See https://github.com/paritytech/cargo-contract/issues/98. | |||
std::env::set_var( | |||
"RUSTFLAGS", | |||
"-C link-arg=-zstack-size=65536 -C link-arg=--import-memory", | |||
"-C link-arg=-zstack-size=65536 -C link-arg=--import-memory -Clto -Clinker-plugin-lto", |
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 believe the -Clto
will override the release profile which we automatically append/merge to the contract's Cargo.toml
. See https://github.com/paritytech/cargo-contract/blob/a8c8589798c05468b41e8125b6c5c3c1194178b6/src/workspace/profile.rs#L22.
I think we might still need to pass Clinker-plugin-lto
directly as an argument, but AFAIU it seems dependent upon -Clto
which is configurable from the [profile.release]
The philosophy we have followed so far is that we should provide sensible defaults for these values, but also allow the user to override them, because the same hardcoded flags might now always produce the optimal contract size for all contracts.
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.
What do you think if we will add it automatically here if lto is on?=)
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.
Clinker-plugin-lto
is not a profile setting https://doc.rust-lang.org/cargo/reference/profiles.html.
What effect does just having Clto
on its own have on the size?
Anyway if this flag is providing such big gains, and doesn't harm any of the other Lto
options then we can possibly hardcode it.
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.
For example that contract takes space:
Without -Clto
and -Clinker-plugin-lto
: Original wasm size: 100.5K, Optimized: 47.9K
With -Clto
: Original wasm size: 68.6K, Optimized: 31.9K
With -Clto
and -Clinker-plugin-lto
: Original wasm size: 67.2K, Optimized: 31.0K
I tried it on contracts from that folder and it causes a positive effect for all of them(but all of them has a heavy logic, except reentrancy
. For reentrancy
it causes a low impact there).
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 measurements for reentrancy
. Also, I removed explicit -Clto
because it causes a compilation error if it is disabled or not aplieable. I tested compiler will use -Clinker-plugin-lto
if he can.
flipper:
Original wasm size: 38.9K, Optimized: 12.5K
Original wasm size: 24.4K, Optimized: 4.5K
Original wasm size: 24.1K, Optimized: 4.2K
flip_on_me:
Original wasm size: 33.8K, Optimized: 10.2K
Original wasm size: 21.5K, Optimized: 2.6K
Original wasm size: 21.2K, Optimized: 2.3K
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.
See comment
@@ -249,7 +249,7 @@ fn exec_cargo_for_wasm_target( | |||
// Currently will override user defined RUSTFLAGS from .cargo/config. See https://github.com/paritytech/cargo-contract/issues/98. | |||
std::env::set_var( | |||
"RUSTFLAGS", | |||
"-C link-arg=-zstack-size=65536 -C link-arg=--import-memory", | |||
"-C link-arg=-zstack-size=65536 -C link-arg=--import-memory -Clinker-plugin-lto", |
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.
If this just does nothing with other lto
settings then should be fine.
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.
Would be good to give the user control over these flags too see: #98
Yea, I saw that issue(I tried to increase the stack size in one moment xDD). I think it is not part of this PR=) |
Do I need to do something to get this PR approved? Or it is okey?=) |
Can you confirm that with different LTO settings (via |
Of course, hence the default. But we still want to provide the flexibility where possible. |
Have you removed the events to get |
Nope, it is only with use-ink/ink#1017 =) |
Ah I forgot, could you add this PR to the Unreleased section of CHANGELOG.md in a quick follow up? |
#364 done=) |
With use-ink/ink#1016 and use-ink/ink#1017 it takes Original wasm size: 31.1K, Optimized: 8.8K(8828) |
-Clinker-plugin-lto
reduced the size of my contractFrom:
Original wasm size: 68.6K, Optimized: 31.9K
To:
Original wasm size: 67.2K, Optimized: 31.0K