Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Substrate loading runtime wasm blob in Native execution mode. #5669

Closed
2 tasks done
nickvikeras opened this issue Sep 10, 2024 · 1 comment
Closed
2 tasks done

Substrate loading runtime wasm blob in Native execution mode. #5669

nickvikeras opened this issue Sep 10, 2024 · 1 comment
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known.

Comments

@nickvikeras
Copy link
Contributor

nickvikeras commented Sep 10, 2024

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Description of bug

Hello,

I recently noticed a bottleneck in my project's unit tests where substrate was loading the wasm blob (takes ~1s) when my test was configured to run in NativeWhenPossible mode. This is happening because here substrate is delegating to the wasm blob to load the runtime version. It looks like we can get the runtime version from the NativeVersion struct field of the NativeElseWasmExecutor struct instead of the wasm blob.

With this small change, the wasm load is avoided and my test (which spins up many nodes as a test network) runs 15s faster:

    diff --git a/client/executor/src/executor.rs b/client/executor/src/executor.rs
    index a3717f4d2..90d05b41c 100644
    --- a/client/executor/src/executor.rs
    +++ b/client/executor/src/executor.rs
    @@ -603,7 +603,7 @@ impl<D: NativeExecutionDispatch> RuntimeVersionOf for NativeElseWasmExecutor<D>
     		ext: &mut dyn Externalities,
     		runtime_code: &RuntimeCode,
     	) -> Result<RuntimeVersion> {
    -		self.wasm.runtime_version(ext, runtime_code)
    +		Ok(self.native_version.runtime_version.clone())
     	}
     }

We are on an older fork of substrate, but it looks like this is still an issue in the new repo as well. https://github.com/paritytech/polkadot-sdk/blob/master/substrate/client/executor/src/executor.rs#L650

Does substrate need to go to the wasm blob to cross-check its version with the native version, or is this just an oversight in the implementation? Would a new "AlwaysNative" ExecutionStrategy be a better solution?

Let me know and I can send a PR.

@nickvikeras nickvikeras added I10-unconfirmed Issue might be valid, but it's not yet known. I2-bug The node fails to follow expected behavior. labels Sep 10, 2024
@nickvikeras
Copy link
Contributor Author

I heard that Native execution mode is actually on the deprecation path, and hence it probably doesn't make sense to introduce such an execution mode. Also, I figured out that enabling compiler optimizations for tests for some crates largely solves the problem anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known.
Projects
None yet
Development

No branches or pull requests

1 participant