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 all instance of inline #1019

Closed
wants to merge 2 commits into from
Closed

Remove all instance of inline #1019

wants to merge 2 commits into from

Conversation

HCastano
Copy link
Contributor

A naive follow up to #1012. I just ran fd -e rs -x sed -i '' -e '/\#\[inline\]/d' and
want to see how contract sizes get affected.

@paritytech-ci
Copy link

paritytech-ci commented Nov 16, 2021

🦑 📈 ink! Example Contracts ‒ Size Change Report 📉 🦑

These are the results of building the examples/* contracts from this branch with cargo-contract 0.15.0:

Δ Original Size Δ Optimized Size Total Optimized Size
accumulator -0.48 K -0.14 K 6.05 K
adder -0.36 K -0.23 K 7.20 K
contract-terminate 1.50 K
contract-transfer +0.49 K -0.01 K 7.72 K
delegator -0.63 K -0.30 K 9.72 K
dns -0.01 K +0.05 K 21.11 K
erc1155 -0.04 K -0.09 K 44.81 K
erc20 +0.08 K -0.05 K 10.12 K
erc721 -0.09 K -0.01 K 35.19 K
flipper -0.03 K -0.02 K 1.99 K
incrementer 1.92 K
multisig -0.16 K -0.31 K 44.53 K
rand-extension +0.48 K +0.27 K 4.94 K
subber -0.36 K -0.23 K 7.20 K
trait-erc20 -0.08 K +0.02 K 26.49 K
trait-flipper -0.02 K -0.02 K 1.81 K
trait-incrementer 1.96 K

Link to the run | Last update: Tue Nov 23 17:42:35 CET 2021

@xgreenx
Copy link
Collaborator

xgreenx commented Nov 16, 2021

It also includes use-ink/cargo-contract#358, I think you also need to check the impact only of use-ink/cargo-contract#358.

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2021

Codecov Report

Merging #1019 (e0d3dee) into master (b2256d9) will increase coverage by 15.47%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1019       +/-   ##
===========================================
+ Coverage   63.52%   78.99%   +15.47%     
===========================================
  Files         246      246               
  Lines        9260     9260               
===========================================
+ Hits         5882     7315     +1433     
+ Misses       3378     1945     -1433     
Impacted Files Coverage Δ
crates/allocator/src/bump.rs 62.50% <ø> (ø)
crates/engine/src/ext.rs 69.07% <ø> (+6.57%) ⬆️
crates/env/src/call/call_builder.rs 0.00% <ø> (ø)
crates/env/src/call/common.rs 0.00% <ø> (ø)
crates/env/src/call/create_builder.rs 0.00% <ø> (ø)
crates/env/src/call/execution_input.rs 76.92% <ø> (ø)
crates/env/src/chain_extension.rs 6.00% <ø> (+6.00%) ⬆️
crates/env/src/topics.rs 100.00% <ø> (ø)
crates/env/src/types.rs 22.22% <ø> (ø)
...odegen/src/generator/as_dependency/call_builder.rs 100.00% <ø> (+100.00%) ⬆️
... and 86 more

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 b2256d9...e0d3dee. Read the comment docs.

@Robbepop
Copy link
Collaborator

While the numbers of our example contracts looks okayish with respect to this PR I would not suggest merging this since removal of all #[inline] is not very thought through. Instead we should try to identify which #[inline] annotated functions should actually not be #[inline]. Note that with respect to inline annotations there is a ton of noise involved. Removing #[inline] for a function foo might end up improving example A but worsen example B at the same time.

@HCastano
Copy link
Contributor Author

While the numbers of our example contracts looks okayish with respect to this PR I would not suggest merging this since removal of all #[inline] is not very thought through. Instead we should try to identify which #[inline] annotated functions should actually not be #[inline]. Note that with respect to inline annotations there is a ton of noise involved. Removing #[inline] for a function foo might end up improving example A but worsen example B at the same time.

Yeah I agree, this was a brute-force follow up more out of curiosity than anything. I've opened #1035 so that we remember to try a more targeted approach in the future.

@HCastano HCastano closed this Nov 23, 2021
@HCastano HCastano deleted the hc-remove-inlines branch November 23, 2021 17:33
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.

5 participants