substrate
substrate copied to clipboard
paritytech
Introduce extra flags for Instance Limits
Description of bug
With the new pooling strategies for wasm instance created, parity deprecated the InstanceReuse strategy (which was the default until 0.9.24)
The issue comes from the hardcoded limits set if the selected strategy is pooling based, a.k.a. the limits set here https://github.com/paritytech/substrate/blob/7d8e5a1e3979cb0fc61123d5cfeec974dfa25334/client/executor/wasmtime/src/runtime.rs#L376-L401
This limits are based on the Kusama runtime, which is probably not very big compared to some parachains. Consequently, a parachain with a runtime bigger than Kusama will highly likely reach this limit and be unable to leverage the new strategies, which is the issue we are facing at composable.
A simple solution would be to introduce extra flags for the limits, such that every team will be able to fine tune the limits w.r.t their runtimes, this would allow us to use the new strategies.
Also, we noticed that the limit was increased here this might not be a sustainable solution, it will be good to introduce a flag here with a default value instead.
[image: A screenshot of the error we are facing]
Steps to reproduce
No response
We could make it configurable at initialization of the wasm executor. Not convinced that we should add CLI flags for this. However, we could also just use the default values of this config which are way higher than the currently set values. CC @koute
It's good to know that people are hitting the limits as it shows that we need to improve the situation here.
However I don't think we should necessarily add new CLI flags to control this. That'll require a little bit of refactoring, but as I've already said elsewhere we can just make the executor autodetect the minimum necessary values for a given runtime and use exactly what's necessary as the limits, and then the problem disappears without putting the burden of picking the right limit on the users. In the immediate/short term, sure, we could just bump the limits significantly.
executor autodetect the minimum necessary values for a given runtime
Sounds like a good idea at hand, however what happens at runtime upgrades? :D
executor autodetect the minimum necessary values for a given runtime
Sounds like a good idea at hand, however what happens at runtime upgrades? :D
The limits get recalculated, the new runtime's compiled, and everyone's happy. (: All of those limits (besides count, but that one can be kept hardcoded and be mitigated in a different way) can essentially be calculated statically based only on the uncompiled raw runtime blob, and they are (in the current implementation) only set per runtime, not globally (although we do hardcode them currently, so only incidentally they are global). So if we have a new runtime we want to run we just calculate the new limits for it, and any other runtime which needs to run in parallel can still run unaffected.
Anyway, I'll queue this up on my TODO list.
A pedantic side note: this is relatively straightforward because the limits are configured per each wasmtime::Engine instance, and we do create a separate Engine for each runtime instance so we can set different limits for each unique runtime as we see fit. There was however a proposal to use a single Engine for all of our runtime instances (unfortunately I can't find the issue right now though) and that could make this slightly more problematic (because then you'd have multiple different runtimes sharing the same Engine), but that'd still be doable if we'd make Engine sharing opportunistic (share the Engine if the new runtime's within the old limits, don't if it isn't; we'd probably have to do this anyway since e.g. the Engine for PVF would need a slightly different configuration anyway). This is of course assuming we'd actually want to limit the number of Engines we create, and I'm not even convinced it'd be worth it anyway.
Bumping this again, The composable runtime includes pallet-ibc, and pallet-cosmswasm (which bundles paritytech/wasmi) which takes our wasm size over these limits and prevents even our local polkadot-launch set up from working as the relay chain nodes fail to instantiate our parachain wasm.
This issue is going to block us from deploying our runtime to production. Please help @bkchr @koute
~~You are free to open a pr to do the changes I proposed above @seunlanlege.~~
Do what @koute said and bump the limits significantly and then you should be fine.
@koute @bkchr this issue still prevent us from using the pooling strategy, forcing us to rely on the deprecated one. Please let me know if #13298 is okay for you
@koute @bkchr this issue still prevent us from using the pooling strategy, forcing us to rely on the deprecated one. Please let me know if #13298 is okay for you
Yeah that's fine.