Wouldn't it be much more clean to just restore the original data section?
This all looks like a big hack.

Wouldn't it be much more clean to just restore the original data section?

I assume you meant restoration of the original data snapshot as well as other components of an instance (basically, cloning an wasm instance), otherwise it would have the same problems as #2051 to some degree.

I think we might have tried to restore/to clean the whole memory instance, but it was taking considerably more time. I think that is the reason why we got the current optimization that attempts at zeroing the (heuristically detected) dirty areas of memory (I think @arkpar might have some info on this).

The proposed approach, at least conceptially (I haven't had a look at the code yet), is very simple, so I think I can repeat it here:

1. Create a pool of instances.
2. On one side of the pool is producers which replenish the pool in background threads.
3. On another side of the pool is consumers - wasm_executor which asks for an instance.

The general assumption behind this is the typical workload is more or less synchronous and thus the latency of wasm runtime execution matters more than actual time spent on preparing these instances. While threading and interthread communications definitely add some overhead, we are lowering the latency on the critical path to almost zero on average. Besides that, it doesn't use any specific feature of a wasm execution engine (wasmi at the moment), so this mechanism can be easily reused for another execution engine (i.e. #2634).

All of this and simplicity of the approach indicated that it is a good path to explore.

Although, it uncovered a problem with wasmi: wasmi in the current state is unusable in multithreaded environment, and I am not happy about it as well. That said, I think the general direction is still worth exploring.

Do you have anything in particular that you don't like in this solution (conceptually)?

Updates:

1. This approach uses a raw thread which is step backwards with respect to
   Add a light client code that compiles in the browser #2416
2. If the inital assumption that copying of whole memory takes too long is not true, then plain cloning of the runtime instance template is actually very tempting because of its simplicity.

At first we had intended for the runtime pool to do more complex actions when adding instances to the pool. Right now it just clones a runtime instance from a "template instance". I measured the clone operation in release mode on dev over 40 blocks and it takes on average about 1 µs on my machine. So it seems rather cheap and I'm wondering if we could maybe even do this synchronously without having the overhead of a separate thread?

If the inital assumption that copying of whole memory takes too long is not true, then plain cloning of the runtime instance template is actually very tempting because of its simplicity.

I'll try it out in a separate branch/PR. This would also save us from having to switch to thread-safe types for wasmi.

My complain was mostly about copying everything ^^ But if there is no better way, okay..

Conceptually I did not look that much into the code. In general you are right, we should not spawn raw threads in here. Then we need to take a look, how we evict old runtimes from the cache. Maybe it makes sense to remove them after some blocks and not directly as we see a new runtime.
And if we already touch this, I would really like to see the global cache variable gone and that the cache is passed in from the outside. Having global variables, was not a good solution from the beginning.

The problem is with the way rust-to-wasm toolchain organizes wasm memory. IIRC it allocates 1MB for the stack and and the data segment follows it. So each new runtime instance requires a 1MB write to memory, which impacts the performance when you have a lot of runtimes created for simple calls into WASM.

I'd really prefer this to be fundamentally fixed by avoiding that huge unnecessary work, rather than working around it. Pooling would waste CPU and will degrade heavily under load. E.g. when you have a lot of RPC requests coming in parallel, which is quite typical in various blockchain applications.

Pooling would waste CPU and will degrade heavily under load. E.g. when you have a lot of RPC requests coming in parallel, which is quite typical in various blockchain applications.

This is really good input. As I mentioned, the assumption was that the node has spare CPU time. If you think that is not the case then this would be an issue.

I have some ideas in mind how to fix itproperly. However, they would require non-trivial amount of modification to the current execution execution engine - wasmi - and for the execution engines that would be introduced in the future (#2634). Opting into this more sophisticated approach would inhibit experimentation as well.

But most importantly, this issue is pressing due to an upcoming audit. This solution seemed to be pretty simple and shouldn't have regressed the performance very much.

That said, I really agree that we need to use a proper solution eventually and I see the proposed solution as a stop-gap.

Would still love to see how it affects real block import performance. I.e. import 10000 testnet blocks from a file. Also please compare CPU load.

I'm closing this PR as we are currently favoring the approach from #2938 and have stopped working on this one. If the other one turns out to be a dead end we can always reopen this PR.