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

perf: Persistent executor #5082

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dima74
Copy link
Contributor

@dima74 dima74 commented Sep 17, 2024

Context

Meta issue: optimizing single peer tps #4727.
It was identified that executor related things takes noticeable amount of time (#3716 (comment)).
Fixes #3716

Solution

Single executor WASM instance will be used for validating all transactions of a block. It gives approximately 10-15% improvement of single peer tps (from 2900 to 3300). However there is a problem with lifetimes and I have to use a hack with std::mem::transmute to bypass borrow checker. Would be glad to hear opinions/suggestions about it.

Review notes (optional)

Primary change is that data stored in wasmtime::Store was changed from CommonState<...> to Option<CommonState<...>>

Checklist

  • I've read CONTRIBUTING.md.
  • (optional) I've written unit tests for the code changes.
  • All review comments have been resolved.
  • All CI checks pass.

@Erigara
Copy link
Contributor

Erigara commented Sep 17, 2024

Not really comfortable with using unsafe.

What is going under the hood is that WasmCache contain inside state transaction with lifetime <'world, 'block, 'state>, but than we actually pass inside StateTransaction with a lifetimes shorter than that with a promise that we will clear state after transaction is validated, right?

@dima74
Copy link
Contributor Author

dima74 commented Sep 17, 2024

Currently WasmCache stores &'world mut StateTransaction<'block, 'state> inside. We create WasmCache before obtaining any StateTransaction, so the first problem is that there is no 'world lifetime when we create WasmCache.

I tried to eliminate 'world lifetime by storing Arc<RefCell<StateTransaction<'block, 'state>>> inside WasmCache. In this case problem will be in categorize_transactions function. Basically:

fn categorize_transactions<'block, 'state>(
    transactions: Vec<AcceptedTransaction>,
    state_block: &'block mut StateBlock<'state>,
) -> Vec<CommittedTransaction> {
    let wasm_cache: WasmCache<'block, 'state> = WasmCache::new();
    validate(tx1, state_block, &mut wasm_cache);
    validate(tx2, state_block, &mut wasm_cache);  // here borrow check error
    ...
}

fn validate(
    tx: AcceptedTransaction,
    state_block: &'block mut StateBlock<'state>,
    wasm_cache: &mut WasmCache<'block, 'state>,
) {
    ...
}

&mut WasmCache<'block, 'state> is invariant over 'block thus calling validate multiple times will fail borrow check

crates/iroha_core/src/smartcontracts/wasm/cache.rs Outdated Show resolved Hide resolved
crates/iroha_core/src/smartcontracts/wasm/cache.rs Outdated Show resolved Hide resolved
@@ -749,6 +762,17 @@ impl<W, S> Runtime<state::CommonState<W, S>> {
}
}

impl<W, S> Runtime<Option<state::CommonState<W, S>>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way you have to repeat impls for the Option<CommonState<...>> is unfortunate... Would it make sense to make all uses of CommonState wrapped into an Option? Or would this be too disruptive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't like it, but using Option<CommonState> everywhere will make code more complex I think

@dima74 dima74 force-pushed the diralik/persistent-executor branch 3 times, most recently from b105b4d to eb45eca Compare October 1, 2024 13:07
Signed-off-by: Dmitry Murzin <[email protected]>
Signed-off-by: Dmitry Murzin <[email protected]>
@dima74 dima74 force-pushed the diralik/persistent-executor branch from eb45eca to c949df8 Compare October 2, 2024 12:17
@dima74
Copy link
Contributor Author

dima74 commented Oct 2, 2024

Rebased after #5113

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

[suggestion] Persistent Executor
4 participants