Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[contracts] Remove stack height limiter wasm instrumentation #12957

Merged
merged 5 commits into from
Dec 22, 2022

Conversation

agryaznov
Copy link
Contributor

We don't really need a stack limiter, because:

  • we use a runtime-resident execution engine,
    hence there is no indeterminism chance caused by different stack heights, which was the case with client-sided engines;
  • it's on the execution engine duty to deal with possible stack overflows,
    in our case it is handled in wasmi;
  • this instrumentation itself is about of quadratic time complexity from the number of function calls the input module contains,
    hence getting rid of it we're saving the runtime resources (and therefore upload_code will take less weight);
  • bonus: this also makes the resulting instrumented wasm blob lighter, therefore eating less storage and requiring less storage deposit from the contract deploy-er, let alone performance improvements to the contract execution.

@agryaznov agryaznov added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Dec 16, 2022
@agryaznov agryaznov changed the title [contracts] Remove stack height limiter from uploaded wasm [contracts] Remove stack height limiter wasm instrumentation Dec 16, 2022
@agryaznov
Copy link
Contributor Author

/cmd queue -c bench-bot $ pallet dev pallet_contracts

@command-bot
Copy link

command-bot bot commented Dec 16, 2022

@agryaznov https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2168849 was started for your command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_contracts. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment /cmd cancel 115-4e56fe3a-e1ba-408d-b26c-0001895cf315 to cancel this command or /cmd cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Dec 16, 2022

@agryaznov Command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_contracts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2168849 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2168849/artifacts/download.

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍 (code-wise; don't know enough to reason about business logic)

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

I resolved the merge conflict by choosing the upstream weight file. This didn't change the weight because the stack height limiter was already disabled (set to 0). This PR merely removed the inactive code.

@athei
Copy link
Member

athei commented Dec 22, 2022

bot merge

@athei athei merged commit acbb91b into master Dec 22, 2022
@athei athei deleted the ag-remove-stack-limiter branch December 22, 2022 17:22
@bkchr
Copy link
Member

bkchr commented Dec 22, 2022

  • we use a runtime-resident execution engine,
    hence there is no indeterminism chance caused by different stack heights, which was the case with client-sided engines;

Is this correct? AFAIK we don't have any deterministic stack metering for runtimes, which should result in having indeterminism for this here as well?

@athei
Copy link
Member

athei commented Dec 22, 2022

We do:

if let Some(DeterministicStackLimit { logical_max, .. }) = semantics.deterministic_stack_limit {
blob = blob.inject_stack_depth_metering(logical_max)?;
}

@bkchr
Copy link
Member

bkchr commented Dec 22, 2022

deterministic_stack_limit: None,

We don't use this for the runtimes right now :P

And there is also this issue which we probably need to fix before: #9298

ltfschoen pushed a commit to ltfschoen/substrate that referenced this pull request Feb 22, 2023
…ech#12957)

* Remove stack height limiter from uploaded wasm

* fix benchmarks

* ".git/.scripts/bench-bot.sh" pallet dev pallet_contracts

Co-authored-by: command-bot <>
Co-authored-by: Alexander Theißen <[email protected]>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…ech#12957)

* Remove stack height limiter from uploaded wasm

* fix benchmarks

* ".git/.scripts/bench-bot.sh" pallet dev pallet_contracts

Co-authored-by: command-bot <>
Co-authored-by: Alexander Theißen <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
Status: Done ✅
Development

Successfully merging this pull request may close these issues.

4 participants