paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

wasm: enable new features #10707

Open pepyakin opened 2 years ago

pepyakin commented 2 years ago

This is a placeholder issue for enabling support of a recently added features into the wasm spec.

Some of these allow leveraging the underlying machine more efficiently and others open up better ways of expressing public API.

Enabling a feature would be just fl

  1. It would be great to have data on how exactly performance is improved. This would help to evaluate how much priority it has for implementation and for the runtime writers user for upgrading.
  2. wasmi does not support pretty much most of the newest features. wasmtime is the primary engine for now, but it would still be good to have the second engine.
  3. The polkadot side should be taken into account. How would PVF execution migrated? Or those features won't hit PVF?
nazar-pc commented 2 years ago

I was looking into this a bit to force enable simd and found https://github.com/paritytech/wasm-instrument/issues/4 is needed before those features can be enabled in Substrate, once that is done, it will be possible to test things with wasmtime by tweaking a just few things.

pepyakin commented 2 years ago

As a potential heads up, LLVM is enabling some default features in clang, see here. I assume rustc is soon to follow (if not already).

We should be ready for it since one day we might build a runtime that won't pass our checks.

As a related note, a similar discussion is happening on the wasmtime side, here https://github.com/bytecodealliance/wasmtime/issues/3969. The consensus is that it does not make sense to perpetually continue supporting these minimal configurations. It's hard to argue with the arguments that are given there, so I think it would be best if we explicitly checked for using those features on our side if needed. That might affect our plans for parity-wasm/wasm-instrument (cc @athei )

cc @koute

athei commented 2 years ago

Yeah we need to tackle https://github.com/paritytech/wasm-instrument/issues/3 soon.

koute commented 2 years ago

There's always the -mcpu=mvp flag we could use to enforce the current baseline featureset; the question is whether that'll be perpetually supported into the future, and whether it will be well supported?

For x86 clang still supports -march=i386 which allows you to generate code for essentially a 37-year old CPU, so is it safe to assume the WASM MVP target won't be quickly dropped? But even if it won't be dropped is it going to be safe to target a baseline most other people don't? The more niche featureset we target the higher the chance to hit codegen bugs, which could be catastrophic in our case.

I think we will probably want to gradually migrate to a newer WASM featureset, but in the short term it shouldn't be a critical issue as long as we force the compiler to not use the newer instructions.

pepyakin commented 2 years ago

Yes, -mcpu=mvp is the short-term solution here, and thanks for mentioning it. The other points are great too.

Some features like multivalue, mutable globals, signext, and co are good to use since they are natural extensions of wasm. Bulk memory extensions are trickier since, e.g., memory.copy does a variable amount of work. This may be a problem for things like metering of runtimes/PVFs. Features that introduce non-determinism that cannot be easily fixed are a no-go.

Given that, I guess the WASM feature set we need won't match -mcpu=generic. And thus, we may still end up in a similar situation of using a more niche feature set.

koute commented 2 years ago

Bulk memory extensions are trickier since, e.g., memory.copy does a variable amount of work. This may be a problem for things like metering of runtimes/PVFs.

That still could be relatively easily metered, I think? Indeed it does variable amount of work, but the work it does is simple, and is highly correlated with the size of the memory region it is passed. Although it most likely won't be exactly linear (it won't reach its maximum throughput unless the buffer's big), but we could just weight it differently based on a few different thresholds or something. I think that should be doable as long as we're careful. (e.g. depending on the exact SIMD instructions used to implement those the performance can vary very widely, so we'll have to make sure that this uses what we think it uses)

pepyakin commented 2 years ago

Sure, did not mean to say that it is insurmountable. Just that it might introduce complications for metering like as you mentioned with attributing the costs but also potentially introducing edge-cases to the metering itself which may warrant just not implementing the proposal (if the cost/benefit analysis does not look too good).

tomaka commented 1 year ago

For what it's worth, it seems that wasmtime doesn't allow disabling the sign-ext feature. Substrate therefore always accepts as valid the runtimes that use it. It thus makes sense to me to allow it.

EDIT: Same for "Non-trapping Float-to-int" and mutable-globs

bkchr commented 1 year ago

Substrate therefore always accepts as valid the runtimes that use it

Substrate currently rejects runtime that use it because of parity-wasm, but there is a plan to enable it, but still reject it: https://github.com/paritytech/polkadot-sdk/issues/15