refactor(event cache): remove an empty events chunk after pushing a gap in sync #4734
Conversation
The linked chunk always starts with an empty events chunk. If we receive a gap from sync, then we will immediately push a gap chunk; in this case, it might be better to replace the events chunk with a gap chunk. This is equivalent to removing the empty events chunk, after pushing back the first one (we can't do it before, otherwise we might get rid of the only chunk in the linked chunk, which breaks the invariant that a linked chunk is never empty).
…y used in testing This isn't useful to keep, since it's only used in testing. Worst case, we can revert this commit in the future.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4734 +/- ##
=======================================
Coverage 86.14% 86.14%
=======================================
Files 291 291
Lines 34308 34316 +8
=======================================
+ Hits 29553 29560 +7
- Misses 4755 4756 +1 ☔ View full report in Codecov by Sentry. |
| @@ -687,7 +694,7 @@ impl<const CAP: usize, Item, Gap> LinkedChunk<CAP, Item, Gap> { | |||
| /// | |||
| /// This returns the next insert position, viz. the start of the next | |||
| /// chunk, if any, or none if there was no next chunk. | |||
| pub fn remove_gap_at( | |||
| pub fn remove_empty_chunk_at( | |||
There was a problem hiding this comment.
Github doesn't allow me to attach a comment at line 693 but the comment of this method has to be updated:
- Remove a gap with the given identifier.
+ Remove a chunk with the given identifier if and only if it is empty.
+
+ A chunk is considered empty if: it contains zero items, or is a gap.| }; | ||
| match chunk.content() { | ||
| ChunkContent::Items(items) if !items.is_empty() => { | ||
| if chunk.is_items() { |
There was a problem hiding this comment.
We know it's an Items chunk already. Remove this condition.
| match chunk.content() { | ||
| ChunkContent::Items(items) if !items.is_empty() => { | ||
| if chunk.is_items() { | ||
| return Err(Error::RemovingNonEmptyItemsChunk { identifier: chunk_identifier }); | ||
| }; | ||
| } | ||
| _ => { | ||
| // Either a gap or an empty items chunk: continue. | ||
| } | ||
| } |
There was a problem hiding this comment.
I think you can rewrite this full block into this:
if chunk.len() > 0 {
return Err(Error::RemovingNonEmptyItemsChunk { … });
}Chunk::len returns 0 for a Gap, or the number of items for an Items.
| let err = linked_chunk.remove_gap_at(ChunkIdentifier(0)).unwrap_err(); | ||
| assert_matches!(err, Error::ChunkIsItems { .. }); | ||
| // Try to remove a chunk that's not empty. | ||
| let err = linked_chunk.remove_empty_chunk_at(ChunkIdentifier(0)).unwrap_err(); |
There was a problem hiding this comment.
Please, can you rename the test from test_remove_gap_at to test_remove_empty_chunk_at?
| // It was the last chunk, so there's no next insert position. | ||
| assert!(next.is_none()); | ||
| assert_items_eq!(linked_chunk, [-] ['a', 'b'] ['l', 'm']); | ||
| assert_eq!(linked_chunk.updates().unwrap().take(), &[RemoveChunk(ChunkIdentifier(4))]); | ||
|
|
||
| // Remove the gap at the beginning. | ||
| let maybe_next = linked_chunk.remove_gap_at(ChunkIdentifier(1)).unwrap(); | ||
| let maybe_next = linked_chunk.remove_empty_chunk_at(ChunkIdentifier(1)).unwrap(); |
There was a problem hiding this comment.
Can you update the test to remove an empty Items chunk too please? Simply change a Gap to an Items in the test data (+ updating inline test comments) and it would be good.
| @@ -687,7 +694,7 @@ impl<const CAP: usize, Item, Gap> LinkedChunk<CAP, Item, Gap> { | |||
| /// | |||
| /// This returns the next insert position, viz. the start of the next | |||
| /// chunk, if any, or none if there was no next chunk. | |||
| pub fn remove_gap_at( | |||
| pub fn remove_empty_chunk_at( | |||
There was a problem hiding this comment.
Please, can you update your patch to include a new check:
LinkedChunk MUST contain at least 2 chunks before removing one. Otherwise the LinkedChunk will be empty, and it violates a constraint.
We can remove this constraint, but it involves a bit more work. Also we don't need for the moment.
| room_events.rchunks().next().and_then(|chunk| { | ||
| match chunk.content() { | ||
| ChunkContent::Items(events) if events.is_empty() => { | ||
| Some(chunk.identifier()) | ||
| } | ||
| _ => None, | ||
| } | ||
| }); |
There was a problem hiding this comment.
Suggestion. Can be rewritten like so:
room_events.rchunks().next().and_then(|chunk| (chunk.is_items() && chunk.len() == 0).then_some(chunk.identifier());(it may involve changing the visibility of Chunk::len() to pub).
Small optimization: when we have an empty events chunk, and we're about to push a gap, remove the empty events chunk. It's the same code as for removing a gap chunk, so not much new code is introduced in the linked chunk code (nice!). This makes back-pagination indicate we've reached the start of the timeline quicker, because now they don't have to load an initial empty chunk.
Also, get rid of
EmptyChunkRule, as it was always set to::Removein the code, and only set to::Keepin tests. In the worst case where we'd find that having::Keepis useful, we can still revert the commit individually in the future.Part of #3280.