paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.78k stars 645 forks source link

PVF: Consider removing `Executor` struct #622

Closed mrcnski closed 11 months ago

mrcnski commented 1 year ago

ISSUE

Overview

This struct doesn't seem that useful now that the rayon threadpool has been removed. It used to look like this. Without that, its methods can be standalone functions that take a Config. This would slightly simplify some of the code. I won't remove it just yet in case it could be useful again later.

From https://github.com/paritytech/polkadot/pull/7246#discussion_r1210646106

eagr commented 11 months ago

https://github.com/paritytech/polkadot-sdk/blob/2b4b33d01f22b1f4037a404597357f398e21224f/polkadot/node/core/pvf/execute-worker/src/lib.rs#L192-L197

Does this call construct a runtime every time? If so, this may be worth doing. Or am I missing anything?

mrcnski commented 11 months ago

Not sure I'm following @eagr. But this issue would be a nice simplification if you wanna take it on. :)

eagr commented 11 months ago

validate_using_artifact() is being invoked in a loop. It in turn calls Executor::execute() which constructs a WasmtimeRuntime every time. It seems to me that the WasmtimeRuntime should be reused to spawn instances rather than being constructed on each iteration, right?

mrcnski commented 11 months ago

Interesting idea @eagr. The runtime construction depends on the artifact, so we can't re-use the same runtime for every job. But there's maybe an idea here of keeping around all runtimes in memory. If they use 30mb of RAM each, a validator could keep 30 (or however many to cover all parachains). However I think the runtime construction is a very small part of execution, and this cache would probably be less useful when pay-as-you-go arrives. @eskimor