Optimized decode of AccountId #1016

xgreenx · 2021-11-15T19:52:59Z

Default decode takes ~300 bytes

Robbepop · 2021-11-15T20:15:06Z

Default decode takes ~300 bytes

Can you tell us the effect on some examples for this? It would be interesting to know why the default is so bad here. Also we could apply this to Hash and Key types as well.

xgreenx · 2021-11-15T20:19:30Z

Yea, sorry. thought that ink-waterfall will show that=)
Tested on latest master with use-ink/cargo-contract#358.

Before change:
Original wasm size: 34.3K, Optimized: 10.6K
-rw-r--r-- 1 501 20 10632 Nov 15 22:18 target/ink/erc20.wasm

After change:
Original wasm size: 33.6K, Optimized: 10.2K
-rw-r--r-- 1 501 20 10242 Nov 15 22:16 target/ink/erc20.wasm

codecov-commenter · 2021-11-15T20:46:00Z

Codecov Report

Merging #1016 (7cf279b) into master (7432565) will decrease coverage by 11.58%.
The diff coverage is 63.63%.

@@             Coverage Diff             @@
##           master    #1016       +/-   ##
===========================================
- Coverage   75.37%   63.78%   -11.59%     
===========================================
  Files         246      246               
  Lines        9258     9267        +9     
===========================================
- Hits         6978     5911     -1067     
- Misses       2280     3356     +1076

Impacted Files	Coverage Δ
crates/env/src/types.rs	`27.27% <50.00%> (+5.05%)`	⬆️
crates/primitives/src/key.rs	`93.50% <100.00%> (+0.08%)`	⬆️
crates/lang/codegen/src/traits.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/lang/codegen/src/generator/env.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/lang/codegen/src/generator/mod.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/lang/codegen/src/enforced_error.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/lang/codegen/src/generator/blake2b.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/lang/codegen/src/generator/storage.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/lang/codegen/src/generator/arg_list.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/lang/codegen/src/generator/contract.rs	`0.00% <0.00%> (-100.00%)`	⬇️
... and 17 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 7432565...7cf279b. Read the comment docs.

cmichi · 2021-11-16T07:24:32Z

Yea, sorry. thought that ink-waterfall will show that=)

We'll merge paritytech/ink-waterfall#16 hopefully today, then it will :-).

xgreenx · 2021-11-16T15:57:51Z

Yea, sorry. thought that ink-waterfall will show that=)

We'll merge paritytech/ink-waterfall#16 hopefully today, then it will :-).

How to trigger the report?=)

paritytech-ci · 2021-11-16T16:09:38Z

🦑 📈 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`			6.20 K
`adder`	-2.35 K	-1.10 K	6.33 K
`contract-terminate`			1.50 K
`contract-transfer`			7.97 K
`delegator`	-1.49 K	-0.88 K	9.16 K
`dns`	-1.24 K	-0.60 K	21.76 K
`erc1155`	-0.21 K	+0.05 K	48.31 K
`erc20`	-0.68 K	-0.39 K	10.24 K
`erc721`	-0.65 K	-0.34 K	36.63 K
`flipper`			2.02 K
`incrementer`			1.94 K
`multisig`	-0.75 K	-0.50 K	47.10 K
`rand-extension`			4.89 K
`subber`	-2.35 K	-1.10 K	6.33 K
`trait-erc20`	-0.69 K	-0.39 K	27.58 K
`trait-flipper`			1.83 K
`trait-incrementer`			1.99 K

Link to the run | Last update: Tue Nov 16 17:09:38 CET 2021

cmichi · 2021-11-16T16:17:11Z

How to trigger the report?=)

We just merged the PR, it's triggered automatically now with every run of the ink-waterfall CI stage in ink! ‒ so every ink! PR where the CI manages to get there (it's one of the last stages).

cmichi · 2021-11-16T16:18:55Z

@xgreenx Btw. the above report already includes the changes you made in use-ink/cargo-contract#358.

xgreenx · 2021-11-16T16:25:02Z

How to trigger the report?=)

We just merged the PR, it's triggered automatically now with every run of the ink-waterfall CI stage in ink! ‒ so every ink! PR where the CI manages to get there (it's one of the last stages).

Last stage... Maybe it is possible to run manually for some PR(for example #1017 )?=D

cmichi · 2021-11-16T16:32:19Z

Last stage... Maybe it is possible to run manually for some PR(for example #1017 )?=D

Not trivially, I guess it's the reward you get once you've fixed all the CI stages before it ;-). But is there anything preventing you from fixing the CI stages at that PR?

xgreenx · 2021-11-16T16:36:37Z

Not trivially, I guess it's the reward you get once you've fixed all the CI stages before it ;-). But is there anything preventing you from fixing the CI stages at that PR?

I'm not sure about the final state of the code, so I'm waiting for some input from you and @Robbepop=) Because it requires removing some logic added before and maybe you have your vision of the code. I described the main idea and results that I got=)

Robbepop

I find it strange that the Decode derive impl and the impls provided by this PR are not optimized to same or similar IR. It would be nice to know why. Some guesses: Special treatment for Copy types, no guaranteed RVO, no guaranteed copy elision etc.

crates/env/src/types.rs

xgreenx · 2021-11-16T17:24:28Z

I find it strange that the Decode derive impl and the impls provided by this PR are not optimized to same or similar IR. It would be nice to know why. Some guesses: Special treatment for Copy types, no guaranteed RVO, no guaranteed copy elision etc.

The default implementation of Decode for the generic array is complex. In wasm, it takes ~360 bytes. I found it during analyzing of objdumped contract and decide to create this change=)

Robbepop · 2021-11-16T17:49:34Z

I find it strange that the Decode derive impl and the impls provided by this PR are not optimized to same or similar IR. It would be nice to know why. Some guesses: Special treatment for Copy types, no guaranteed RVO, no guaranteed copy elision etc.

The default implementation of Decode for the generic array is complex. In wasm, it takes ~360 bytes. I found it during analyzing of objdumped contract and decide to create this change=)

Looks like we should rather fix and optimise the generic array implementation than all of its dependents. We could use MaybeUninit or even array::map for this. Shouldnt be too hard.

xgreenx · 2021-11-16T18:05:50Z

Looks like we should rather fix and optimise the generic array implementation than all of its dependents. We could use MaybeUninit or even array::map for this. Shouldnt be too hard.

I agree that better to do a fix for the general case. But it means that general implementation will be not so general because it requires some restrictions like Copy trait, or Default or Size or another=) And it can affect someone who uses parity-scale-codec. So this decision is up to you(Parity team), I only highlighted that can be optimized=D

Robbepop · 2021-11-16T19:43:23Z

Looks like we should rather fix and optimise the generic array implementation than all of its dependents. We could use MaybeUninit or even array::map for this. Shouldnt be too hard.

I agree that better to do a fix for the general case. But it means that general implementation will be not so general because it requires some restrictions like Copy trait, or Default or Size or another=) And it can affect someone who uses parity-scale-codec. So this decision is up to you(Parity team), I only highlighted that can be optimized=D

If MaybeUninit is used no further restrictions need to be put in place. There are some crates for safe and efficient array initialization.

xgreenx · 2021-11-16T20:00:51Z

If MaybeUninit is used no further restrictions need to be put in place. There are some crates for safe and efficient array initialization.

I will try it. If everything is okay, then I will create a pull request=)

BTW, if it is not hard then it's strange why it is not implemented from the beginning)

xgreenx · 2021-11-17T13:24:26Z

I will try it. If everything is okay, then I will create a pull request=)

BTW, if it is not hard then it's strange why it is not implemented from the beginning)

@Robbepop I implemented idea with MaybeUninit. After that change the size of Erc20 decreased:
Original wasm size: 34.0K, Optimized: 10.4K(10389)

But a native implementation(from this PR) still is better=) Original wasm size: 33.5K, Optimized: 10.2K(10163)

If you agree to use usable feature(like negative-impls or specialization), we can specify the implementation for u8=D

xgreenx · 2021-11-18T21:09:10Z

This change is implemented on parity-scale-codec level=) paritytech/parity-scale-codec#299

Optimized decode of AccountId

2e40d9b

Added custom Decode for Key and Hash

7cf279b

xgreenx mentioned this pull request Nov 16, 2021

Use -Clinker-plugin-lto flag during Wasm build use-ink/cargo-contract#358

Merged

Robbepop reviewed Nov 16, 2021

View reviewed changes

crates/env/src/types.rs Show resolved Hide resolved

xgreenx mentioned this pull request Nov 18, 2021

Optimised the Decode::decode for [T; N] paritytech/parity-scale-codec#299

Merged

xgreenx closed this Nov 18, 2021

xgreenx mentioned this pull request Dec 8, 2021

OpenBrush follow-up Grant update Supercolony-net/Open-Grants-Program#8

Merged

0xMarkian mentioned this pull request Dec 8, 2021

OpenBrush follow-up Grant update w3f/Grants-Program#735

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimized decode of AccountId #1016

Optimized decode of AccountId #1016

xgreenx commented Nov 15, 2021 •

edited

Loading

Robbepop commented Nov 15, 2021

xgreenx commented Nov 15, 2021 •

edited

Loading

codecov-commenter commented Nov 15, 2021

cmichi commented Nov 16, 2021

xgreenx commented Nov 16, 2021

paritytech-ci commented Nov 16, 2021

cmichi commented Nov 16, 2021 •

edited

Loading

cmichi commented Nov 16, 2021

xgreenx commented Nov 16, 2021 •

edited

Loading

cmichi commented Nov 16, 2021

xgreenx commented Nov 16, 2021 •

edited

Loading

Robbepop left a comment

xgreenx commented Nov 16, 2021

Robbepop commented Nov 16, 2021 •

edited

Loading

xgreenx commented Nov 16, 2021 •

edited

Loading

Robbepop commented Nov 16, 2021

xgreenx commented Nov 16, 2021

xgreenx commented Nov 17, 2021 •

edited

Loading

xgreenx commented Nov 18, 2021

Optimized decode of AccountId #1016

Optimized decode of AccountId #1016

Conversation

xgreenx commented Nov 15, 2021 • edited Loading

Robbepop commented Nov 15, 2021

xgreenx commented Nov 15, 2021 • edited Loading

codecov-commenter commented Nov 15, 2021

Codecov Report

cmichi commented Nov 16, 2021

xgreenx commented Nov 16, 2021

paritytech-ci commented Nov 16, 2021

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

cmichi commented Nov 16, 2021 • edited Loading

cmichi commented Nov 16, 2021

xgreenx commented Nov 16, 2021 • edited Loading

cmichi commented Nov 16, 2021

xgreenx commented Nov 16, 2021 • edited Loading

Robbepop left a comment

Choose a reason for hiding this comment

xgreenx commented Nov 16, 2021

Robbepop commented Nov 16, 2021 • edited Loading

xgreenx commented Nov 16, 2021 • edited Loading

Robbepop commented Nov 16, 2021

xgreenx commented Nov 16, 2021

xgreenx commented Nov 17, 2021 • edited Loading

xgreenx commented Nov 18, 2021

xgreenx commented Nov 15, 2021 •

edited

Loading

xgreenx commented Nov 15, 2021 •

edited

Loading

cmichi commented Nov 16, 2021 •

edited

Loading

xgreenx commented Nov 16, 2021 •

edited

Loading

xgreenx commented Nov 16, 2021 •

edited

Loading

Robbepop commented Nov 16, 2021 •

edited

Loading

xgreenx commented Nov 16, 2021 •

edited

Loading

xgreenx commented Nov 17, 2021 •

edited

Loading