Ensure EventIds and Positions are unique within a LinkedChunk and that Events are synchronized across LinkedChunks

[mgoldenberg]

This pull request enforces three properties on implementations of EventCacheStore through integration tests.

1. An Event should not exist within a LinkedChunk more than once - i.e., it should occupy only a single Position.
2. Each Position within a LinkedChunk should only be occupied by a single Event.
3. When changes are made to the content of an Event in one LinkedChunk, these changes should be reflected in all LinkedChunks which contain an instance of that Event.

Enforcing these properties required adding and updating integration tests, as well as some modifications to each of the EventCacheStore implementations - i.e., in-memory, SQLite, and IndexedDB.

Changes

Integration Tests

A new integration test was added to EventCacheStoreIntegrationTests - i.e., test_linked_chunk_push_items. This test enforces that implementations conform to properties (1), (2), and (3) above when processing an Update::PushItems.

Furthermore, test_linked_chunk_replace_item has been updated to ensure that implementations adhere to property (3) above when processing an Update::ReplaceItem.

In-Memory

In the in-memory implementation of EventCacheStore, the following steps were taken to ensure compliance with the properties above.

• Checks were added when processing Update::PushItems and Update::ReplaceItem to ensure compliance with (1) and (2) above.
• Positions are removed when processing Update::RemoveItem, Update::DetachLastItems, and Update::Clear to ensure compliance with (2) above.
• Event content is updated across LinkedChunks when processing Update::PushItems and Update::ReplaceItem to ensure compliance with (3) above.

Performance

This results in a significant performance penalty, as ensuring these properties often requires iterating over many items. That being said, I don't think performance is much of a concern for this implementation, as it is used primarily for testing.

SQLite

In the SQLite implementation of the EventCacheStore, changes were mininmal as it was already compliant with properties (2) and (3) above. All that was required was to establish a uniqueness constraint on the LinkedChunkId and the EventId in the event_chunks table to ensure compliance with (1) above.

These changes were discussed in #6844.

IndexedDB

The IndexedDB implementation was already compliant with properties (1) and (2) above. It did not, however, properly update Events across LinkedChunks when processing Update::PushItems and Update::ReplaceItem.

In order to do this efficiently, a new (non-unique) index was added to the EVENTS object store which tracks the EventId of an Event, regardless of the LinkedChunk in which it exists. This allows all Events in all LinkedChunks with the same EventId to be retrieved in a single query.

Additionally, IndexeddbEventCacheStoreTransaction::{add_event, put_event} were updated to ensure that they retrieved all Events in the store with the provided EventId and updated with the provided content. This ensured compliance with property (3) above.

Performance

While there don't seem to be any benchmarks for the IndexedDB implementation, this will certainly result in a performance penalty. This is because processing Update::PushItems and Update::ReplaceItem now updates all instances of an Event across all LinkedChunks, which is not possible to do without reading them all into memory, modifying them, and then writing them back to the database.

---

• I've documented the public API changes in the appropriate changelog files (see Writing changelog entries).
• This PR was made with the help of AI.

Signed-off-by: Michael Goldenberg m@mgoldenberg.net

[codecov]

Codecov Report

❌ Patch coverage is 92.98246% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.14%. Comparing base (b479a6f) to head (e18f0dd).
⚠️ Report is 35 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...s/matrix-sdk-common/src/linked_chunk/relational.rs	95.91%	2 Missing ⚠️
crates/matrix-sdk-sqlite/src/event_cache_store.rs	75.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6872      +/-   ##
==========================================
+ Coverage   90.10%   90.14%   +0.04%     
==========================================
  Files         407      407              
  Lines      115745   115794      +49     
  Branches   115745   115794      +49     
==========================================
+ Hits       104294   104385      +91     
+ Misses       7540     7500      -40     
+ Partials     3911     3909       -2     

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

[codspeed-hq]

Merging this PR will degrade performance by 92.99%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 5 regressed benchmarks
✅ 45 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	BASE	HEAD	Efficiency
❌	Simulation	Event cache room updates[memory][room count: 100]	5.3 s	1,391 s	-99.62%
❌	Simulation	Linked chunk writing [memory store][1000]	7.1 ms	128.3 ms	-94.5%
❌	Simulation	Event cache room updates[memory][room count: 10]	527.6 ms	8,388.4 ms	-93.71%
❌	Simulation	Event cache room updates[memory][room count: 1]	53.1 ms	163.1 ms	-67.47%
❌	Simulation	Linked chunk writing [memory store][100]	711.5 µs	1,814.7 µs	-60.79%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

---

_{Comparing mgoldenberg:indexeddb-event-unique-in-linked-chunk (e18f0dd) with main (1d4bc12)}

[Hywan]

I think we are good, except maybe one test that could be improved, thoughts?

[Hywan]

You are missing the case where you are pushing an existing event in a free position.

Normally we have the Deduplicator for that, but we need to ensure the storage will have a uniqueness constraint too.

[mgoldenberg]

I think this is handled by operation (5) in my comment above, no?

[Hywan]

I'm also sad about the performance regression. I'm afraid it could impact us. Do you feel we can do something about it?

[mgoldenberg]

I'm also sad about the performance regression. I'm afraid it could impact us. Do you feel we can do something about it?

As far as I can tell, the regression is only in the memory store, correct? I haven't looked into it yet, but I think we could probably add another data structure to help speed up some of the searches. The downside, of course, is that there will be more data redundancy.

What do you think? I'm happy to look into it!

[Hywan]

As far as I can tell, the regression is only in the memory store, correct? I haven't looked into it yet, but I think we could probably add another data structure to help speed up some of the searches. The downside, of course, is that there will be more data redundancy.

Ah yes, of course, not a problem then, I don't really care about the memory store, it's mostly for testing purposes.

[mgoldenberg]

So, it looks like the benchmarks for the event cache have not been run for any of the more recent commits. Things seems to fail in another job and then cancel the remaining benchmarks. Are you able to restart those, @Hywan?

[Hywan]

Jobs restarted.

[Hywan]

Can you rebase your fixup commits please? They look good to me.

[Hywan]

Looks good to me, just need to rebase.