Multi-process support: Implement finer-grained in-memory caching for the crypto store #2624
Assignees
Labels
Comments
This was referenced Sep 26, 2023
bnjbvr
added a commit
that referenced
this issue
Oct 9, 2023
This adds a new `StoreTransaction` type, that wraps a `StoreCache` and a `Store`. The idea is that it will allow write access to the `Store` (and maintains the cache at the same time), while the `Store::cache` method will only give read-only access to the store's content. Another new data type is introduced, `PendingChanges`, that reflects `Changes` but for fields that are properly maintained in the `StoreCache` and that one can write in the `StoreTransaction`. In the future, it wouldn't be possible to use `save_pending_changes` from the outside of a `StoreTransaction` context. The layering is the following: - `Store` wraps the `DynCryptoStore`, contains a reference to a `StoreCache`. - When read-only access is sufficient, one can get a handle to the cache with `Store::cache()`. - When a write happens, then one can create a `StoreTransaction` (later, only one at a time will be allowed, by putting the `StoreCache` behind a `RwLock`; this has been deferred to not make the PR grow too much). - Any field in the `StoreCache` will get a method to get a reference to the cached thing: it will either load from the DB if not cached, or return the previously cached value. - Any field that can be written to will get a method to get a mutable reference in the `StoreTransaction`: it will either load from the cache into a `PendingChanges` scratch pad, or return the scratchpad temporary value. - When a `StoreTransaction::commit()` happens, fields are backpropagated into the DB *and* the cache. Then, this `StoreTransaction` is used to update a `ReadOnlyAccount` in multiple places (and usage of `ReadOnlyAccount` is minimized so as not to require a transaction or cache function call as much as possible). With this, the read-only account only exists transiently, and it's only stored long-term in the cache. Followup PRs include: - making the `ReadOnlyAccount` not cloneable - remove inner mutability from the `ReadOnlyAccount` - add a `RwLock` on the `StoreTransaction` Part of #2624 + #2000. --- * crypto: Replace some uses of `ReadOnlyAccount` with `StaticAccountData` and identify tests * crypto: introduce `StoreTransaction` to modify a `ReadOnlyAccount` * crypto: introduce `save_pending_changes`, aka `save_changes` v2 * crypto: Start using `StoreTransaction`s to save the account, get rid of `Store::save_account` + `Account::save` * crypto: use `StoreTransaction` to save an account in `keys_for_upload` * crypto: use `StoreTransaction` and the cache in more places * crypto: remove `Account` from the `Changes` \o/ * crypto: remove last (test-only) callers of `Store::account()` * crypto: move `ReadOnlyAccount` inside the cache only * crypto: use `ReadOnlyAccount` and `Account` in fewer places whenever we can use `StaticAccountData` in place. * crypto: make tests rely less on OlmMachine * crypto: Don't put the `ReadOnlyAccount` behind a RwLock just yet + clippy
This was referenced Oct 13, 2023
bnjbvr
changed the title
|
Just discussed it with @BillCarsonFr. In the quest of killing UTDs, the crypto team is mostly interested by the |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently, our cross-process lock blows up the entire crypto state (
OlmMachine) whenever we notice another process has written to underlying storage. This was implemented because we really wanted a working implementation for decrypting notifications on iOS, but this really is a shortcut. In particular, short-lived crypto procedures (like verification etc) could be interrupted if another process holds the lock in the middle; or we don't currently have any locking when writing a new message (and this might happen in another process too, see also #1960).This issue is about fixing and implementing that properly, by having some sort of "cross-process lock guard" representing all the crypto-store data that can be put in a cache. Acquiring this lock may invalidate the cache if another process has written to the underlying database, transparently for the users of the lock. For the point of view of any user of that cache, it's "just" another
asyncfallible operation.The idea is to introduce a new
CryptoStoreCachedata structure, which maintains two properties:The overall "migration" process is thus the following:
CryptoStoreCacheAlternatively, we could also drop in-memory caching support for some of these fields, if the performance impact is deemed low enough.
Once that's done for all those fields, the final step would be to move back the crypto-store lock into the
OlmMachineand simplify public APIs again.A good testing strategy should also be established to make sure we can invalidate the cache at any time, and that it would not fiddle with any of the function calls that may be running in the background.
Tasklist
Bootstrapping
ReadOnlyAccountnotCloneable (crypto: Remove inner mutable shared state fromAccount#2710)ReadOnlyAccount(crypto: Remove inner mutable shared state fromAccount#2710)ReadOnlyAccountandAccount✨ #2680Account#2660RwLockon theStoreCache(crypto: Remove inner mutable shared state fromAccount#2710)Migrating
Take each field that's cached and move it to the transactional API and store cache.
Sessionsinto the cacheGroupSessionCache/InboundGroupSession/OutboundGroupSessionThe final touch
OlmMachineand don't blow up the entire OlmMachine whenever the lock observed another process wrote to the DBBackups::room_keys_for_room_stream()method, replace it with theOlmMachine::store()::room_keys_received_stream().Backups::secret_send_event_handlerwithOlmMachine::store()::secrets_stream().OlmMachinethat notifies us about the state of our private cross-signing keys.The text was updated successfully, but these errors were encountered: