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

Remove always from inline to allow compiler decide that to do #1012

Merged

Conversation

xgreenx
Copy link
Collaborator

@xgreenx xgreenx commented Nov 10, 2021

It's funny, but removing always with combination use-ink/cargo-contract#358, reduces the size of Erc20 contract in master from 31.4K to 27.4K

polkadot address: 1nNaTpU9GHFvF7ZrSMu2CudQjXftR8Aqx58oMDgcuoH8dKe

… form:

Original wasm size: 66.7K, Optimized: 30.2K
To:
Original wasm size: 63.9K, Optimized: 27.4K
@HCastano
Copy link
Contributor

This looks promising! We should wait until the ink-waterfall pipeline is fixed though, since there we can see how this PR changes the sizes of all the example contracts instead of just the ERC-20 one.

@codecov-commenter
Copy link

Codecov Report

Merging #1012 (843b06d) into master (c7cc828) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1012      +/-   ##
==========================================
- Coverage   78.86%   78.84%   -0.03%     
==========================================
  Files         245      245              
  Lines        9246     9246              
==========================================
- Hits         7292     7290       -2     
- Misses       1954     1956       +2     
Impacted Files Coverage Δ
crates/env/src/chain_extension.rs 0.00% <ø> (ø)
...odegen/src/generator/as_dependency/call_builder.rs 100.00% <ø> (ø)
...ng/codegen/src/generator/trait_def/call_builder.rs 100.00% <ø> (ø)
crates/lang/src/codegen/dispatch/execution.rs 0.00% <ø> (ø)
crates/lang/src/result_info.rs 100.00% <ø> (ø)
crates/primitives/src/key.rs 93.42% <ø> (ø)
crates/storage/src/collections/stash/storage.rs 60.97% <ø> (ø)
crates/storage/src/memory.rs 100.00% <ø> (ø)
crates/storage/src/traits/impls/prims.rs 96.92% <ø> (ø)
...ates/storage/src/collections/hashmap/fuzz_tests.rs 97.87% <0.00%> (-2.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7cc828...843b06d. Read the comment docs.

Copy link
Collaborator

@cmichi cmichi left a comment

Choose a reason for hiding this comment

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

Very nice discovery!

Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

So here are the changes in size of our example contracts compared to master. Note that all sizes are in KB, and - means that it is smaller with this PR applied.

contract-terminate,0.0,0.0
rand-extension,-0.055000305,-0.12400007
incrementer,0.035999298,-0.06700015
erc1155,-63.791008,-62.012
trait-erc20,-2.6740036,-2.7460003
dns,-0.10300064,-0.22099876
delegator,0.38000107,0.15100002
multisig,-14.578995,-13.453999
subber,0.20700073,0.14900017
flipper,0.069000244,-0.03699994
erc20,-2.675003,-2.7460003
trait-flipper,0.024999619,-0.06700003
contract-transfer,0.0,0.0
trait-incrementer,-0.20399857,-0.32500005
erc721,-3.8440018,-3.9720001
adder,0.20700073,0.14900017
accumulator,0.20699883,0.14900017

Notice the dramatic size reduction to the ERC-1155 example 🤯. We should really do some digging and figure out why this "simple" change has produced such a great improvement there.

@cmichi
Copy link
Collaborator

cmichi commented Nov 12, 2021

@xgreenx We want to give you a tip for all the great stuff you have contributed now and in the past. We're happy to have you as part of our ecosystem!

Could you edit your description of this PR and add either one of those?

polkadot address: <SS58 Address>

or

kusama address: <SS58 Address>

@xgreenx
Copy link
Collaborator Author

xgreenx commented Nov 12, 2021

@xgreenx We want to give you a tip for all the great stuff you have contributed now and in the past. We're happy to have you as part of our ecosystem!

Glad to hear it=) Thank you, it is part of our grant, so you don't need to worry about it(=

So here are the changes in size of our example contracts compared to master. Note that all sizes are in KB, and - means that it is smaller with this PR applied.

Hmm, contracts that use builder have a bad impact(delegator,0.38000107,0.15100002), maybe we should keep inlining for them. I will try it.

@xgreenx
Copy link
Collaborator Author

xgreenx commented Nov 12, 2021

Hmm, contracts that use builder have a bad impact(delegator,0.38000107,0.15100002), maybe we should keep inlining for them. I will try it.

I tried to return #[inline(always)] back in some places to reduce the size of the delegator back. Seems that use-ink/cargo-contract#358 should cover that case. Because the size is the same with #[inline(always)] and #[inline] if I build with use-ink/cargo-contract#358. So I think we can merge it=)

@cmichi
Copy link
Collaborator

cmichi commented Dec 16, 2021

/tip medium

@substrate-tip-bot
Copy link

A medium tip was successfully submitted for xgreenx (1nNaTpU9GHFvF7ZrSMu2CudQjXftR8Aqx58oMDgcuoH8dKe on polkadot).

https://polkadot.js.org/apps/#/treasury/tips

xgreenx added a commit to Supercolony-net/ink that referenced this pull request Feb 8, 2022
…-ink#1012)

It funny, but removing `always`, reduces the size of `Erc20` contract form: 
Original wasm size: 66.7K, Optimized: 30.2K
To:
Original wasm size: 63.9K, Optimized: 27.4K
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.

4 participants