Sync keyvaluestore#6153
Conversation
Instruction Count Benchmark Results
Deterministic metrics — reproducible across runs (34 benchmarks)
Cache-dependent metrics — expect fluctuations between runs (34 benchmarks)
|
|
|
||
| /// A synchronous in-memory store. | ||
| #[derive(Clone)] | ||
| pub struct SyncMemoryStore { |
There was a problem hiding this comment.
I don't see any difference with MemoryStore. Seems like you could just implement all traits for MemoryStore.
There was a problem hiding this comment.
Yes, the MemoryStore was anyway sync, so nest a SyncMemoryStore in it.
| fn max_stream_queries(&self) -> usize { | ||
| self.max_stream_queries | ||
| } |
There was a problem hiding this comment.
I don't think we need this in a SyncReadableKeyValueStore implementation.
| let map = self | ||
| .map | ||
| .read() | ||
| .expect("SyncMemoryStore lock should not be poisoned"); |
There was a problem hiding this comment.
nit: .unwrap() is accepted in this case
| Ok(values) | ||
| } | ||
|
|
||
| async fn find_key_values_by_prefix( |
There was a problem hiding this comment.
Could all the async methods simply call their synchronous sister instead of duplicating the code?
There was a problem hiding this comment.
You are right, the code is simplified.
| } | ||
| } | ||
|
|
||
| impl SyncReadableKeyValueStore for SyncMemoryStore { |
There was a problem hiding this comment.
Note that RocksDb would be a happy SyncReadableKeyValueStore (although I don't quite have a usecase for it right now).
MathieuDutSik
force-pushed
the
sync_keyvaluestore
branch
3 times, most recently
from
04cb35f to
340796a
Compare
| Self::post_load(context, &[]) | ||
| } else { | ||
| let keys = Self::pre_load(&context)?; | ||
| let values = context.store().read_multi_values_bytes(&keys).await?; |
There was a problem hiding this comment.
Is this .await the only difference?
Would it make sense have a single variable maybe_await that is either "" or ".await" instead? Then we could deduplicate this and this line would become:
| let values = context.store().read_multi_values_bytes(&keys).await?; | |
| let values = context.store().read_multi_values_bytes(&keys)#maybe_await?; |
There was a problem hiding this comment.
In that case, it is not the only difference. There is also
use linera_views::{context::Context as _, store::ReadableKeyValueStore as _};vs
use linera_views::{context::SyncContext as _, store::SyncReadableKeyValueStore as _};|
|
||
| let has_pending_changes_fn = match mode { | ||
| ViewMode::Async => quote! { | ||
| async fn has_pending_changes(&self) -> bool { |
There was a problem hiding this comment.
…and a maybe_async would allow deduplicating this and others.
There was a problem hiding this comment.
Yes, we can do that.
| const TEST_MEMORY_MAX_STREAM_QUERIES: usize = 10; | ||
|
|
||
| /// The number of streams for the sync test store | ||
| const SYNC_MEMORY_MAX_STREAM_QUERIES: usize = 10; |
There was a problem hiding this comment.
What kind of streams? Isn't this only used in the async case?
There was a problem hiding this comment.
Yes, we had a leakage of abstraction:
- Yes, the
SYNC_MEMORY_MAX_STREAM_QUERIESis not relevant to theSyncMemoryStore. - But the
MemoryStorewas nesting aSyncMemoryStorehence that structure.
Addressed.
| /// A synchronous in-memory store. | ||
| /// | ||
| /// This is the primary type holding the actual fields and sync trait | ||
| /// implementations, since the underlying storage (BTreeMap behind |
There was a problem hiding this comment.
| /// implementations, since the underlying storage (BTreeMap behind | |
| /// implementations, since the underlying storage (`BTreeMap` behind |
(Or "B-tree map", I guess.)
| { | ||
| /// The type of the key-value store used by this context. | ||
| type Store: SyncReadableKeyValueStore | ||
| + SyncWritableKeyValueStore |
There was a problem hiding this comment.
That's the only difference compared to Context… I also noticed that some (all?) of the view types are actually identical to their async versions.
I'm wondering if it would be better to, instead of having sync versions of all these structs and traits, just add sync (i.e. blocking) methods to the existing structs and traits?
In some impls (memory, RocksDB) the async methods would call the more natural sync ones; in others (ScyllaDB, DynamoDB) it would be the other way round? For views, some of the methods would still have two full implementations, of course, but e.g. all the mutation methods would not, since they are already sync!
There was a problem hiding this comment.
Using blocking method like .blocking_wait() is exactly what we have now in the linera-sdk.
I think the benchmarks demonstrate that this has costs.
We certainly could provide Sync and async functionalities for all the storage. However, I do not think it is a good idea since it oversteps the crucial question: What is actually being used?
And the situation is the following:
- For protocol code, async code is most natural.
- For smart contract code, sync code is most natural.
I think that is a better approach than looking at having the most general code.
| #[allocative(bound = "C, T: Allocative")] | ||
| pub struct SyncLazyRegisterView<C, T>( | ||
| /// The inner async lazy register view whose state and logic we reuse. | ||
| pub(crate) LazyRegisterView<C, T>, |
There was a problem hiding this comment.
Should this be private? Do we access it elsewhere?
| #[allocative(bound = "C, T: Allocative")] | ||
| pub struct SyncLogView<C, T>( | ||
| /// The inner async log view whose state and logic we reuse. | ||
| pub(crate) LogView<C, T>, |
use it for the contract code.
MathieuDutSik
force-pushed
the
sync_keyvaluestore
branch
from
4166b57 to
bd1498e
Compare
3 participants
Motivation
The
KeyValueStoreis an async library that is being used for implementing the views. When used forprotocol code that is fine. However, contract code is actually sync, so using
KeyValueStoreis actuallyusing the wrong tools.
Fixes #3026
Proposal
Introduce a
SyncKeyValueStorethat does the operation in a sync way. The translation is direct.After this the justification for having
async fn execute_operation(_)collapses, and we can instead writefn execute_operation(_). That asyncness was anyway entirely artificial since we have in fact a.block_wait()in the
linera-sdkcode.A similar change cannot be made for the
Servicetrait since it uses theasync-graphqlfunctionality, which doesnot have a sync variant.
Test Plan
CI
The test
test_wasm_end_to_end_matching_engine_benchmarkruns in 762.849ms in main and in 683.950916ms for this branch.Release Plan
That can actually be backported to
testnet_conwaysince all the artificial.blocking_wait()callsare inside of the
linera-sdk, not of the protocol.But of course, the documentation would have to be changed.
Links
None