Remove all #[inline.*]s

[agryaznov]

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]

Let's check the contracts size change report.

[HCastano]

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

[agryaznov]

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

addressed in #1667

[HCastano]

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

[agryaznov]

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]

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]

Waterfall is back #1667, merge master

[agryaznov]

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]

🦑 📈 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]

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

[codecov-commenter]

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]

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.

[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]

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]

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]

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]

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:

1. It lacks context. I think it is better explained on use.ink. Just putting it there might even lead to confusion.
2. 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.
3. 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.
4. 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]

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:

1. 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.
2. 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]

1. 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.

2. 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]

1. 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.

2. 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.

[cmichi]

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

[ascjones]

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

[agryaznov]

@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]

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