Concurrent preloader for contracts

[olonho]

To minimize deserialization delays we introduce mechanism of pipelined constructing
WASM modules before execution.

On simulated tests (with disabled in-memory instance cache) we get the following results.

time cargo test --release  --color=always --test test_vm_runner test_vm_run_sequential --no-fail-fast --manifest-path runtime/near-vm-runner/Cargo.toml -- --format=json --exact -Z unstable-options --show-output
time cargo test --release  --color=always --test test_vm_runner test_vm_run_preloaded --no-fail-fast --manifest-path runtime/near-vm-runner/Cargo.toml -- --format=json --exact -Z unstable-options --show-output

Sequential execution (0ms cache delay):

real	0m1.152s
user	0m0.934s
sys	0m0.241s

Preloaded execution (0ms cache delay):

real	0m0.810s
user	0m1.202s
sys	0m0.367

Sequential execution (5ms cache delay):

real	0m2.726s
user	0m1.277s
sys	0m0.305s

Preloaded execution (5ms cache delay):

real	0m1.021s
user	0m1.133s
sys	0m0.309s

So with realistic cache delays we get almost 3x speedup.

[ailisp]

what is the reason of handle need to wrap in Some?

[olonho]

Handle may be invalid, and I want that explicit, not by magical value.

[ailisp]

Looks like this would never be error because it's constructed by:

             result.push(ContractCallPrepareResult { handle: Some(index), error: None });

Prepare error would happen here:

     tx.send(VMCallData { result }).unwrap();

and are saved in self.prepared (Vec<CallInner>). so i think to process error, this should refer to self.prepared[prepared.handle]?

[olonho]

Indeed, made it somewhat simpler.

[ailisp]

Nice! I think the benefit is significant. a few nit comment

[matklad]

Seems like we can simplify this to just

Suggested change

	pub struct ContractCallPrepareResult {
	pub handle: Option<usize>,
	pub error: Option<VMError>,
	}
	pub struct ContractCallPrepareResult {
	rx: Receiver<VMCallData>,
	}

that is, we don't need CallInner and the prepared vector, PrepareResult itself could carry all the data necessary for resolution.

[olonho]

Nope, don't want to expose implementation details (such as VM implementation dependencies) in public API of preloader.

[matklad]

But the rx can be private, no?

[matklad]

That's interesting! In principle, we could move this error to prepare stage. But that would be a bad idea, because we need to preserve semantics of sequential execution. More generally, prepare call should never fail, all errors should be moved to the moment we actually run the call.

[olonho]

Also current semantics of preloader is that we preload the contract, and select method to execute later on.

[matklad]

Hm, wait, why we need these here?

[olonho]

Without it doesn’t compile, IIRC. Does it for you?

[matklad]

Ah, right, thats' because we add a test to the root package. Naively, I'd expect the test to be within near-vm-runner crate, like this:

diff --git a/Cargo.lock b/Cargo.lock
index 032d7eb9..60f5dd02 100644
Binary files a/Cargo.lock and b/Cargo.lock differ
diff --git a/Cargo.toml b/Cargo.toml
index b19e030e..1dd38eb4 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -63,10 +63,6 @@ near-chain = { path = "./chain/chain" }
 
 node-runtime = { path = "./runtime/runtime" }
 
-near-vm-logic = { path = "./runtime/near-vm-logic" }
-near-vm-errors = { path = "./runtime/near-vm-errors" }
-near-vm-runner = { path = "./runtime/near-vm-runner" }
-
 near-jsonrpc = { path = "./chain/jsonrpc" }
 near-network = { path = "./chain/network" }
 
diff --git a/runtime/near-vm-runner/src/tests.rs b/runtime/near-vm-runner/src/tests.rs
index 34d6636a..7cbaf3cf 100644
--- a/runtime/near-vm-runner/src/tests.rs
+++ b/runtime/near-vm-runner/src/tests.rs
@@ -1,5 +1,6 @@
 mod error_cases;
 mod invalid_contracts;
+mod contract_preload;
 mod rs_contract;
 mod ts_contract;
 
diff --git a/tests/test_vm_runner.rs b/runtime/near-vm-runner/src/tests/contract_preload.rs
similarity index 96%
rename from tests/test_vm_runner.rs
rename to runtime/near-vm-runner/src/tests/contract_preload.rs
index 1cd42463..3ece2215 100644
--- a/tests/test_vm_runner.rs
+++ b/runtime/near-vm-runner/src/tests/contract_preload.rs
@@ -1,7 +1,6 @@
 use near_primitives::hash::hash;
 use near_primitives::runtime::fees::RuntimeFeesConfig;
 use near_vm_logic::{ProtocolVersion, VMConfig, VMContext, VMKind, VMOutcome};
-use near_vm_runner::{run_vm, ContractCallPrepareRequest, ContractCaller, VMError};
 
 use near_primitives::borsh::BorshSerialize;
 use near_primitives::types::CompiledContractCache;
@@ -13,10 +12,13 @@ use std::sync::{Arc, Mutex};
 use std::thread::sleep;
 use std::time::Duration;
 
+use crate::{run_vm, ContractCallPrepareRequest, ContractCaller, VMError};
+
+
 const TEST_CONTRACT_1: &'static [u8] =
-    include_bytes!("../runtime/near-vm-runner/tests/res/test_contract_rs.wasm");
+    include_bytes!("../../tests/res/test_contract_rs.wasm");
 const TEST_CONTRACT_2: &'static [u8] =
-    include_bytes!("../runtime/near-vm-runner/tests/res/test_contract_ts.wasm");
+    include_bytes!("../../tests/res/test_contract_ts.wasm");
 
 fn default_vm_context() -> VMContext {
     return VMContext {

But I don't actually know the rule we use to decide if we want to put a test into the top-level tests directory, or into a specific crate.

[olonho]

Actually that's exactly how it used to be, and you suggested to move it here in earlier comments. I'm kind of OK with either approach, but keep moving it forward and backward doesn't make much sense.

[matklad]

Ouch, I see now that my suggestion was ambiguous, sorry about that.

Originally, the test was in (from the root of repository)

• /runtime/near-vm-runner/tests/test_vm_runner.rs
• it should be moved to /runtime/near-vm-runner/src/tests/test_vm_runner.rs (hence my suggestion to move it to src/tests, and hence the diff above)
• at the moment, it is at /tests/test_vm_runner.rs

[olonho]

OK, done