indexeddb: Shrink values in inbound_group_sessions store, improving performance all around

[andybalaam]

Fixes #3057.

Change the way we store values in Indexed DB for the crypto store, specifically in the inbound_group_sessions store. Reduce the size of the stored values and replace arrays of numbers with base 64 strings. The rationale and some implementation ideas are documented in #3057. (TL;DR: small values are faster, base64 is faster.)

This change:

• Introduces a MaybeEncrypted type in matrix_sdk_indexeddb::crypto_store::indexeddb_serializer that encapsulates the fact that we serialize differently if the session is encrypted with a store cipher. (Previously, we just serialized an array of numbers in both cases. This was actually quite confusing since these arrays of numbers were holding encoded versions of very different values.) This enum is untagged in Serde to avoid increasing the size of Indexed DB records.
• Adds matrix_sdk_store_encryption::EncryptedValueBase64, which works the same way as EncryptedValue, but stores arrays of numbers as base 64.
• Adds a migration to the new schema to matrix_sdk_indexeddb::crypto_store::migrations, including adding the backed_up_to property, so we can support a fix for #26892 (see this comment) without doing another migration.

I also wrote a performance test that runs in a web browser. In the checked-in version, it runs for 2K records, but when I manually increased the number on my machine to 200K, I got these results:

Inserting 200000 records with v8 schema took 216,745ms.
Inserting 200000 records with v10 schema took 34,875ms.

Counting 200000 records with v8 schema took 11,786ms.
Counting 200000 records with v10 schema took 469ms.

So the speedup is impressive.

[codecov]

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (18eefcf) 83.72% compared to head (e3da325) 83.68%.
Report is 20 commits behind head on main.

Files	Patch %	Lines
crates/matrix-sdk-store-encryption/src/lib.rs	68.75%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3073      +/-   ##
==========================================
- Coverage   83.72%   83.68%   -0.05%     
==========================================
  Files         222      222              
  Lines       23357    23389      +32     
==========================================
+ Hits        19556    19573      +17     
- Misses       3801     3816      +15     

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

[BillCarsonFr]

Great work, thx!
LGTM to me. I agree that it's worth it to include the future backed_up_to field at this point.

[Hywan]

It looks excellent to me. I've a couple of really tiny feedback, but overall, great work!

[poljar]

Oh no: #718.

[dkasak]

Great work with the speed-up!

I'm a bit late to the party, but what role does base64-encoding the payload serve in this? It seems that the trick is to replace an array with lots of values with a single value (a string), which could've just as well been a JSON string. Which would mean the base64 step only serves to add overhead on top of this?

The only way the base64 encoding would make sense is if we opted for a more compact binary representation of the array of numbers (which I think was the ultimate goal of #718 there which poljar linked).

[andybalaam]

Oh no: #718.

@poljar hmm. I think we could move the JSON-specific stuff into the indexeddb crate if we wanted to?

[andybalaam]

Great work with the speed-up!

I'm a bit late to the party, but what role does base64-encoding the payload serve in this? It seems that the trick is to replace an array with lots of values with a single value (a string), which could've just as well been a JSON string. Which would mean the base64 step only serves to add overhead on top of this?

The only way the base64 encoding would make sense is if we opted for a more compact binary representation of the array of numbers (which I think was the ultimate goal of #718 there which poljar linked).

@dkasak the base64 encoding is to transform an array of bytes into a string, because I found that strings are much faster than arrays of numbers to store inside Indexed DB. When you say a JSON string, do you mean treat the bytes as characters directly? That would result in invalid UTF-8, which is not allowed in JSON JavaScript, right?

[poljar]

Oh no: #718.

@poljar hmm. I think we could move the JSON-specific stuff into the indexeddb crate if we wanted to?

Yeah, maybe, will think about it.

[dkasak]

When you say a JSON string, do you mean treat the bytes as characters directly? That would result in invalid UTF-8, which is not allowed in JSON JavaScript, right?

What I mean is using serde_json::to_string (which produces an UTF-8 encoded string representation of the JSON) instead of serde_json::to_vec (which produces a vector of bytes of the same representation). And then skipping the base64 encoding step altogether.

[andybalaam]

We are already encoding to JSON using to_string the question is what are we encoding? Previously it was a Vec<u8> and now it's a String, which contains base64.

[andybalaam]

If I remember correctly, the Vec<u8> we have is a pickled version of an InboundGroupSession.

[dkasak]

Just to write down the conclusion: after a quick chat with @andybalaam, only the unencrypted case unnecessarily base64-encodes the payload before storing. The encrypted case a couple of lines above does what is expected, so that's why we were talking past each other.