dan-da
commented
6 months ago Regarding ArchivalMmr and the Mmr trait
Decoupling ArchivalMmr from the (non-async) Mmr trait seems complex. That trait is used in a number of places, such as:
MutatorSetKernel<MMR: Mmr<Hash>>
MsMembershipProof::batch_update_from_addition()
RemovalRecord::batch_update_from_addition()
mutator_set::insert_mock_item()
mutator_set::remove_mock_item()MutatorSetKernel is the most entangled. It is defined as:
pub struct MutatorSetKernel<MMR: Mmr<Hash>> {
pub aocl: MMR,
pub swbf_inactive: MMR,
pub swbf_active: ActiveWindow,
}It is instantiated in other locations as both ArchivalMmr (async) and MmrAccumulator (sync, from twenty_first). It has several complex methods, and is non-trivial.
Presently, I have ArchivalMmr implementing Mmr trait by spawning a new OS thread for each trait-method call which creates a new tokio runtime, which calls the ArchivalMmr async method that actually implements the functionality (and interacts with storage layer). Example:
fn append(&mut self, new_leaf: Digest) -> MmrMembershipProof<H> {
std::thread::scope(|s| {
s.spawn(|| {
let runtime = tokio::runtime::Runtime::new().unwrap();
runtime.block_on(self.append_async(new_leaf))
})
.join()
.unwrap()
})
}This works, but seems quite inefficient. Unfortunately though the only way I know of to call an async method from a sync method already executing in an async runtime is to spawn another thread with a new runtime.
Worse though than inefficient, is that this actually makes the async calling fn block until the thread is finished. So for ArchivalMmr, this is no better (actually worse) than before this PR with regards to blocking on storage calls. Since ArchivalMmr is used heavily by MutatorSetKernel, this seems a big problem.
However, It may be adequate for merging this PR, and then I can optimize/improve in a follow-up.
Optimizing: Getting rid of the thread::spawn()
Brainstorming approaches.
-
An improvement, though not a solution, would be to create a long-lived thread for each ArchivalMmr instance with a 2-way message channel. So instead of creating a thread per method call, we would pass tasks and results back and forth. This eliminates the cost of creating an OS thread plus tokio runtime for each method call. Unfortunately however it still means that ArchivalMmr methods block on storage, and our goal is to avoid that.
-
An (obvious?) approach is to make a new MmrAsync trait and a new MmrAccumulatorAsync that also uses it. That should work and might be the best solution, but it forces code to become async that isn't presently, and doesn't really need to be. It just becomes forced into it because of implementing or using the Mmr trait. This also gets messy because implementors of various traits from tasm-lib are involved.
- Do not use any Mmr trait and keep MmrAccumulator sync. This would require that MutatorSetKernel can somehow accomodate both sync and async. It could mean adding a MutatorSetKernelAsync that duplicates a lot of logic. Or it could mean that we define it something like:
pub enum MmrType {
ArchivalMmr,
MmrAccumulator,
}
pub struct MutatorSetKernel {
pub aocl: MmrType,
pub swbf_inactive: MmrType,
pub swbf_active: ActiveWindow,
}The difficulty here is that all MutatorSetKernel methods must use a ton of match statements like:
match self.aocl {
MmrType::ArchivalMmr(m) => m.count_leaves().await,
MmrType::MmrAccumulator(m) => m.count_leaves(),
}This is unwieldy. (though probably could be made less ugly with a macro)
Both approaches 2 and 3 require MutatorSetKernel to become async, which means that anything using it must also be async. Approach 3 allows MmrAccumulator to remain sync, is the primary advantage, at cost of making MutatorSetKernel more complex.
- A final approach I considered would impl ArchivalMmr with a Vec
instead of a StorageVec . So it would not be database backed, but would instead load all data from DB at creation, store pending writes, and persist back on request. This should allow ArchivalMmr trait methods to remain sync. Async methods would exist for loading and persisting data.
The drawback is that all data must be loaded up-front and remain in mem. I'm unsure of how much data we are talking about, but my guess is that it may be huge eventually, and not possible to load/store all of it in RAM.
Specifically, ArchivalMmr is used for MutatorSetKernel::aocl and MutatorSetKernel::swbf_inactive fields.
All this considered, approach 2 seems to me the most straight-forward and the one I intend to pursue.
aszepieniec
This is a first cut at making the storage layer async in neptune-core.
I am making it a draft PR because I have a few remaining todos to polish things up:
and storage[1] benches from twenty-first, modified as needed (easy)impl Mmr for ArchivalMmr. done, see writeup.stretch goal:
[1] storage benches put aside for now because they would require async support in divan which appears to be in progress, or else a re-write without divan.
Present status: