Remove all `#[inline.*]`s #1666

agryaznov · 2023-02-20T21:36:52Z

This is a reincarnation of #1019 to close #1035 .

These attributes were placed there as a result of some particular test being ran just once, like e.g. in this PR, and probably in some others, all following the same approach. Although those tests could had shown somewhat improvement on contracts sizes, such approach to optimization is in-sustainable in its root.

Both #[inline] and #[inline(always)] are just hints to the compiler, which inlines functions based on its internal heuristics. As said heuristics change over time, it makes no sense to code against what compiler does in a particular case on a particular run. Rather, we should code against what compiler is allowed to do in general.

Given link-time optimization is turned on by default when building contracts, those attributes should not really affect the compilation result in general.

Therefore it's nothing really wrong with removing all inlines with:

find . -name "*.rs" -type f -exec sed -i '/^.*\#\[inline.*\]/d' {} \;

agryaznov · 2023-02-20T21:37:20Z

Let's check the contracts size change report.

HCastano · 2023-02-21T01:42:09Z

@agryaznov the waterfall isn't running anymore, so you're gonna have to check manually and update us on this

agryaznov · 2023-02-22T07:44:32Z

the waterfall isn't running anymore, so you're gonna have to check manually and update us on this

addressed in #1667

HCastano · 2023-02-23T22:52:55Z

@agryaznov since the waterfall (still) isn't working, can you provide us with manual sizes?

agryaznov · 2023-02-24T09:56:38Z

since the waterfall (still) isn't working, can you provide us with manual sizes?

It's been two years from the last attempt to do what this PR is intended to. I don't think we are in a rush with this and I don't think "manual sizes" is a right thing to do whatsoever. Let's hold on until waterfall is fixed, which seems to be a more urgent issue anyway.

HCastano · 2023-02-28T02:48:14Z

since the waterfall (still) isn't working, can you provide us with manual sizes?

It's been two years from the last attempt to do what this PR is intended to. I don't think we are in a rush with this and I don't think "manual sizes" is a right thing to do whatsoever. Let's hold on until waterfall is fixed, which seems to be a more urgent issue anyway.

Getting the sizes manually is not that hard. If you take a look at what the waterfall is doing for size calculations it's basically running a single script in a loop (one for master and once for the PR) and just formatting the results nicely into a table.

ascjones · 2023-02-28T11:35:40Z

Waterfall is back #1667, merge master

agryaznov · 2023-02-28T13:12:38Z

Getting the sizes manually is not that hard. If you take a look at what the waterfall is doing for size calculations it's basically running a single script in a loop (one for master and once for the PR) and just formatting the results nicely into a table.

The point is not about how's that hard or not. It's about using representative reproducible benchmarks, to compare a taken effect in a sustainable way. Manual testing was taken to justify adding those attributes. Let's not make it a vicious practice.

paritytech-cicd-pr · 2023-02-28T14:01:42Z

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

These are the results when building the integration-tests/* contracts from this branch with cargo-contract 4.0.0-alpha-acc02b1 and comparing them to ink! master:

Contract	Upstream Size (kB)	PR Size (kB)	Diff (kB)	Diff (%)	Change
basic-contract-caller	2.998	3.282	0.284	9.47298	📈
basic-contract-caller/other-contract	1.331	1.477	0.146	10.9692	📈
call-builder-return-value	8.734	8.857	0.123	1.40829	📈
call-runtime	1.77	1.941	0.171	9.66102	📈
conditional-compilation	1.204	1.347	0.143	11.8771	📈
contract-terminate	1.087	1.322	0.235	21.6191	📈
contract-transfer	1.446	1.762	0.316	21.8534	📈
custom-allocator	7.39	7.581	0.191	2.58457	📈
dns	7.313	7.45	0.137	1.87338	📈
e2e-call-runtime	1.057	1.356	0.299	28.2876	📈
e2e-runtime-only-backend	1.633	1.914	0.281	17.2076	📈
erc1155	14.113	14.699	0.586	4.1522	📈
erc20	6.572	7.112	0.54	8.21668	📈
erc721	9.547	9.99	0.443	4.6402	📈
events	4.816	4.967	0.151	3.13538	📈
flipper	1.387	1.533	0.146	10.5263	📈
incrementer	1.217	1.363	0.146	11.9967	📈
lang-err-integration-tests/call-builder-delegate	2.321	2.597	0.276	11.8914	📈
lang-err-integration-tests/call-builder	4.875	5.284	0.409	8.38974	📈
lang-err-integration-tests/constructors-return-value	1.772	1.913	0.141	7.95711	📈
lang-err-integration-tests/contract-ref	4.363	4.482	0.119	2.72748	📈
lang-err-integration-tests/integration-flipper	1.565	1.712	0.147	9.39297	📈
mapping-integration-tests	2.975	3.173	0.198	6.65546	📈
mother	9.499	9.767	0.268	2.82135	📈
multi-contract-caller	5.977	6.111	0.134	2.24193	📈
multi-contract-caller/accumulator	1.09	1.242	0.152	13.945	📈
multi-contract-caller/adder	1.672	1.822	0.15	8.97129	📈
multi-contract-caller/subber	1.693	1.844	0.151	8.91908	📈
multisig	21.406	22.235	0.829	3.87275	📈
payment-channel	5.544	6.342	0.798	14.3939	📈
sr25519-verification	0.865	0.764	-0.101	-11.6763	📉
static-buffer	1.411	1.547	0.136	9.63855	📈
trait-dyn-cross-contract-calls	2.491	2.76	0.269	10.7989	📈
trait-dyn-cross-contract-calls/contracts/incrementer	1.3	1.442	0.142	10.9231	📈
trait-erc20	6.956	7.413	0.457	6.56987	📈
trait-flipper	1.204	1.347	0.143	11.8771	📈
trait-incrementer	1.365	1.508	0.143	10.4762	📈
upgradeable-contracts/delegator	2.902	3.097	0.195	6.7195	📈
upgradeable-contracts/delegator/delegatee	1.368	1.516	0.148	10.8187	📈
upgradeable-contracts/set-code-hash	1.46	1.628	0.168	11.5068	📈
upgradeable-contracts/set-code-hash/updated-incrementer	1.439	1.609	0.17	11.8138	📈
wildcard-selector	2.619	2.493	-0.126	-4.811	📉

Link to the run | Last update: Tue Oct 17 17:39:42 CEST 2023

cmichi · 2023-02-28T14:09:07Z

So no changes in gas, but the contract sizes got bigger.

codecov-commenter · 2023-03-01T13:36:48Z

Codecov Report

Merging #1666 (75ac5f4) into master (6b572a7) will increase coverage by 17.99%.
The diff coverage is n/a.

❗ Current head 75ac5f4 differs from pull request most recent head 9b81949. Consider uploading reports for the commit 9b81949 to get more accurate results

@@             Coverage Diff             @@
##           master    #1666       +/-   ##
===========================================
+ Coverage   52.93%   70.93%   +17.99%     
===========================================
  Files         219      205       -14     
  Lines        6780     6409      -371     
===========================================
+ Hits         3589     4546      +957     
+ Misses       3191     1863     -1328

Files	Coverage Δ
crates/allocator/src/bump.rs	`86.77% <ø> (+0.83%)`	⬆️
crates/engine/src/ext.rs	`68.36% <ø> (-3.39%)`	⬇️
crates/env/src/call/call_builder.rs	`0.00% <ø> (ø)`
crates/env/src/call/common.rs	`0.00% <ø> (ø)`
crates/env/src/call/create_builder.rs	`2.66% <ø> (ø)`
crates/env/src/call/execution_input.rs	`76.31% <ø> (-1.19%)`	⬇️
crates/env/src/chain_extension.rs	`6.38% <ø> (+6.38%)`	⬆️
crates/env/src/topics.rs	`100.00% <ø> (ø)`
crates/env/src/types.rs	`0.00% <ø> (-9.10%)`	⬇️
...odegen/src/generator/as_dependency/call_builder.rs	`100.00% <ø> (+100.00%)`	⬆️
... and 15 more

... and 150 files with indirect coverage changes

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

agryaznov · 2023-03-01T18:01:07Z

So no changes in gas, but the contract sizes got bigger.

@cmichi I've tweaked optimization options a little, which resulted in size decrease for most of the example contracts. See the updated report above.

We still get slightly enlarged sizes of the following contracts:

dns
erc20
erc721
erc1155
multisig

I have some ideas on how we could try to make them smaller, too. Will propose them in a follow-up PR if succeeded.

integration-tests/trait-incrementer/Cargo.toml

athei

It looks like the sizes go down for most contracts and up for some. So it is more or less neutral with a tendency to positive. Which is enough to remove the attributes.

I rather have the optimizer make this decision using its global view of the code than follow gut feeling.

I will approve once you revert your profile changes to the examples.

agryaznov · 2023-03-01T18:38:56Z

I will approve once you revert your profile changes to the examples.

cargo-contract's default opt-level = 'z' does not make always the best results. And that's why it's better to measure it and customize for the cases where the s setting works better, which is done here.

athei · 2023-03-01T18:44:33Z

I don't think we should customize here. We should just switch the default to s in cargo-contract if data suggests that this is the better default.

Customizing here is brittle. Small changes can make z better. We should not care about this here.

agryaznov · 2023-03-01T18:54:38Z

I don't think it should be the same option for all the contracts. Contracts are different in their design, and different optimization options for them is fine. If we change a contract we can do this easy benchmarking to ensure which of the two options (z vs s) work better for it. The point is, for the 5 contracts listed in this comment, z works better. We can of course change the default in cargo-contract, but still why not to customize it for special contracts?

Also, some contract authors would optimize for size, others for speed. This is why llvm gives these options to them to decide. By setting non-default options for at least some of the contracts, we give ink! contracts developers an explicit example of such an optimization.

athei · 2023-03-01T19:34:17Z

I agree that we should teach contract authors that changing the opt-level can be a a tool that is at their disposal. But I don't think that cluttering our integration tests with this config is the best way to do so:

It lacks context. I think it is better explained on use.ink. Just putting it there might even lead to confusion.
We don't need to optimize the sizes here as this code is not meant to be deployed. We just need a frame of reference when making changes but not the optimal size.
Maintenance overhead. If changing the log level is on the table we suddenly need to start to mess around with it when making changes to not cause a regression. It also generates a bit of copy paste.
It prevents us from making an proper assessment of changes: Is this regression OK (cause it is just < 1KB since the compiler decides to inline bunch of stuff) or do we need to fix some inherent brittleness in our codegen? This point is a bit weak.

If we still decide to override we should only override opt-level since the rest is already set. Reason is that I think we should minimize config. I am not sure if cargo-contract is able to do that or if a profile is always overridden as a whole.

agryaznov · 2023-03-01T19:47:40Z

Alright, the argument that these contracts are not examples anymore, they became the integration tests, never deployed anywhere except a single substrate-contracts-node during CI, is accepted.

And the clearing the profile config to just have a single non-default option, is fine by me.

So the questions left are:

Should we change default of opt-level to s in cargo-contract?
Motivation: most of the ~~example~~ integration-tests contracts have smaller size with this.
Should we customize the opt options in the ink-examples repo, for the small group of popular contracts, as those ones exposed as part of ink! learning materials? Combined with covering this topic in use.ink docs, of course.

@cmichi @ascjones your opinion is welcomed

athei · 2023-03-01T21:02:25Z

Yes. s seems to be the more common level and is more balanced. Since it also doesn't perform worse (even better in our examples) we should switch.
I think this is fair. But we should make sure that we only override the options that are not set by default (opt-level) in this case. This might require a change to cargo-contract first (maybe it already works I didn't check). We should also add a comment when overriding.

This is just my opinion. @cmichi @ascjones should whey in here before we make a decision.

ascjones · 2023-03-02T15:03:36Z

Agree that if s is better overall for sizes of the contracts then we should change the default. However it would be good to prove this as a standalone PR (separate from the [inline] changes, which would either involve something like Allow specifying opt-level as CLI arg cargo-contract#1000 to control the opt-level for all contracts, or just a draft PR which simulates it by changing specifying s in all example manifests just to generate the size diff.
Agree that this is a good idea,

But we should make sure that we only override the options that are not set by default

I assume you mean we should only override those values which differ from the defaults.

The behaviour is that the defaults are only used if not specified in [profile.release].

So if you only specify opt-level, the override will be used, for the rest the defaults will be used.

This reverts commit 33d0279.

cmichi · 2023-04-12T13:13:53Z

@agryaznov Could you resolve the merge conflicts? We can merge right after that.

ascjones · 2023-10-17T11:47:46Z

@agryaznov resolve conflicts, merge master and you will get the measurements again with #1916

agryaznov · 2023-10-17T16:15:05Z

@agryaznov resolve conflicts, merge master and you will get the measurements again with #1916

Thanks! the CI job updated the results to the comment above.

( tl;dr: almost all contracts sizes increased with the change ); )

agryaznov · 2023-10-17T16:36:49Z

Given link-time optimization is turned on by default when building contracts, those attributes should not really affect the compilation result in general.

so this initial hypothesis proved to be wrong by made measurements, hence closing this PR

remove all #[inline.*]

f4965ab

agryaznov requested review from cmichi, ascjones, HCastano and SkymanOne as code owners February 20, 2023 21:36

agryaznov added 2 commits February 28, 2023 15:15

Merge branch 'master' into ag-remove-inline

edfa60d

fmt

34eafe8

tweak optimization options in profiles

33d0279

tweak options, iteration 2

75ac5f4

cmichi reviewed Mar 1, 2023

View reviewed changes

integration-tests/trait-incrementer/Cargo.toml Outdated Show resolved Hide resolved

athei reviewed Mar 1, 2023

View reviewed changes

ascjones mentioned this pull request Mar 2, 2023

Allow specifying opt-level as CLI arg use-ink/cargo-contract#1000

Open

agryaznov added 2 commits March 3, 2023 16:06

Revert "tweak optimization options in profiles"

ca802d3

This reverts commit 33d0279.

Merge branch 'master' into ag-remove-inline

f499b31

athei approved these changes Mar 3, 2023

View reviewed changes

agryaznov mentioned this pull request Mar 3, 2023

CI: add job for comparing contracts sizes use-ink/ink-examples#9

Closed

cmichi approved these changes Apr 12, 2023

View reviewed changes

Merge branch 'master' into ag-remove-inline

9b81949

agryaznov closed this Oct 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove all `#[inline.*]`s #1666

Remove all `#[inline.*]`s #1666

agryaznov commented Feb 20, 2023 •

edited

Loading

agryaznov commented Feb 20, 2023

HCastano commented Feb 21, 2023

agryaznov commented Feb 22, 2023

HCastano commented Feb 23, 2023

agryaznov commented Feb 24, 2023

HCastano commented Feb 28, 2023

ascjones commented Feb 28, 2023

agryaznov commented Feb 28, 2023

paritytech-cicd-pr commented Feb 28, 2023 •

edited

Loading

cmichi commented Feb 28, 2023

codecov-commenter commented Mar 1, 2023 •

edited

Loading

agryaznov commented Mar 1, 2023

athei left a comment

agryaznov commented Mar 1, 2023

athei commented Mar 1, 2023

agryaznov commented Mar 1, 2023 •

edited

Loading

athei commented Mar 1, 2023

agryaznov commented Mar 1, 2023 •

edited

Loading

athei commented Mar 1, 2023

ascjones commented Mar 2, 2023

cmichi commented Apr 12, 2023

ascjones commented Oct 17, 2023

agryaznov commented Oct 17, 2023 •

edited

Loading

agryaznov commented Oct 17, 2023

Remove all #[inline.*]s #1666

Remove all #[inline.*]s #1666

Conversation

agryaznov commented Feb 20, 2023 • edited Loading

agryaznov commented Feb 20, 2023

HCastano commented Feb 21, 2023

agryaznov commented Feb 22, 2023

HCastano commented Feb 23, 2023

agryaznov commented Feb 24, 2023

HCastano commented Feb 28, 2023

ascjones commented Feb 28, 2023

agryaznov commented Feb 28, 2023

paritytech-cicd-pr commented Feb 28, 2023 • edited Loading

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

cmichi commented Feb 28, 2023

codecov-commenter commented Mar 1, 2023 • edited Loading

Codecov Report

agryaznov commented Mar 1, 2023

athei left a comment

Choose a reason for hiding this comment

agryaznov commented Mar 1, 2023

athei commented Mar 1, 2023

agryaznov commented Mar 1, 2023 • edited Loading

athei commented Mar 1, 2023

agryaznov commented Mar 1, 2023 • edited Loading

athei commented Mar 1, 2023

ascjones commented Mar 2, 2023

cmichi commented Apr 12, 2023

ascjones commented Oct 17, 2023

agryaznov commented Oct 17, 2023 • edited Loading

agryaznov commented Oct 17, 2023

Remove all `#[inline.*]`s #1666

Remove all `#[inline.*]`s #1666

agryaznov commented Feb 20, 2023 •

edited

Loading

paritytech-cicd-pr commented Feb 28, 2023 •

edited

Loading

codecov-commenter commented Mar 1, 2023 •

edited

Loading

agryaznov commented Mar 1, 2023 •

edited

Loading

agryaznov commented Mar 1, 2023 •

edited

Loading

agryaznov commented Oct 17, 2023 •

edited

Loading