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

export: Add a streamed way of getting room keys #3144

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

andybalaam
Copy link
Member

This is part of my work on reducing the memory usage of exporting all our room keys #2914.

It's small part: we offer a streamed method on OlmMachine called export_room_keys_stream that is similar to export_room_keys except it returns a Stream, meaning the caller does not need to have all the keys they requested in memory at the same time.

Inside its implementation, it uses CryptoStore::get_inbound_group_sessions, which returns a Vec, so it doesn't (yet) prevent unbounded memory use.

In a test I did with 10K keys, when I used this method and serialised the result directly to JSON without holding it in an intermediate Vec I saw a few MB of memory savings in Element Web during export.

So there is some immediate benefit, but possibly more importantly, this just feels like the right interface to offer: especially when we are exporting all keys, we don't want to be forced to hold them all in a Vec as export_room_keys does. In future, some PR like #3060 will allow us to stream properly from the database, and not hold all keys in an intermediate Vec as we do now.

I plan to follow this up with a PR in matrix-rust-sdk-crypto-wasm that realises the memory saving I referred to above.

@andybalaam andybalaam requested a review from a team as a code owner February 20, 2024 12:37
@andybalaam andybalaam requested review from poljar and removed request for a team February 20, 2024 12:37
@andybalaam andybalaam force-pushed the andybalaam/export-streaming-notall branch 2 times, most recently from 1cf0e30 to 72d45e8 Compare February 20, 2024 12:44
Copy link

codecov bot commented Feb 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f0354d1) 83.85% compared to head (496ce6b) 83.86%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3144   +/-   ##
=======================================
  Coverage   83.85%   83.86%           
=======================================
  Files         231      231           
  Lines       23856    23860    +4     
=======================================
+ Hits        20005    20009    +4     
  Misses       3851     3851           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Let's avoid adding more methods to the OlmMachine especially if they are clearly storage related for which we already expose the OlmMachine::store().

crates/matrix-sdk-crypto/src/machine.rs Outdated Show resolved Hide resolved
@andybalaam andybalaam force-pushed the andybalaam/export-streaming-notall branch from 1fdae24 to 496ce6b Compare February 20, 2024 14:27
@poljar poljar merged commit dcf0069 into main Feb 20, 2024
34 checks passed
@poljar poljar deleted the andybalaam/export-streaming-notall branch February 20, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants