Skip to content

Commit 48b8e99

Browse files
committed
feat(sdk): Optimise how insert_gap_at works when inserting at first position.
Imagine we have this linked chunk: ```rust assert_items_eq!(linked_chunk, ['a'] [-] ['b', 'c'] ['d', 'e', 'f']); ``` Before the patch, when we were running: ```rust let position_of_d = linked_chunk.item_position(|item| *item == 'd').unwrap(); linked_chunk.insert_gap_at((), position_of_d)?; ``` it was taking `['d', 'e', 'f']` to split it at index 0, so `[]` + `['d', 'e', 'f']`, and then was inserting a gap in between, thus resulting into: ```rust assert_items_eq!(linked_chunk, ['a'] [-] ['b', 'c'] [] [-] ['d', 'e', 'f']); ``` The problem is that it creates a useless empty chunk. It's a waste of space, and rapidly, of computation. When used with `EventCache`, this problem occurs every time a backpagination occurs (because it executes a `replace_gap_at` to insert the new item, and then executes a `insert_gap_at` if the backpagination contains another `prev_batch` token). With this patch, the result of the operation is now: ```rust assert_items_eq!(linked_chunk, ['a'] [-] ['b', 'c'] [-] ['d', 'e', 'f']); ``` The `assert_items_eq!` macro has been updated to be able to assert empty chunks. The `test_insert_gap_at` has been updated to test all edge cases.
1 parent fa5ce1d commit 48b8e99

File tree

1 file changed

+87
-44
lines changed

1 file changed

+87
-44
lines changed

crates/matrix-sdk/src/event_cache/linked_chunk.rs

Lines changed: 87 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,30 @@ impl<Item, Gap, const CAP: usize> LinkedChunk<Item, Gap, CAP> {
197197
.chunk_mut(chunk_identifier)
198198
.ok_or(LinkedChunkError::InvalidChunkIdentifier { identifier: chunk_identifier })?;
199199

200+
// If `item_index` is 0, we don't want to split the current items chunk to
201+
// insert a new gap chunk, otherwise it would create an empty current items
202+
// chunk. Let's handle this case in particular.
203+
//
204+
// Of course this optimisation applies if there is a previous chunk. Remember
205+
// the invariant: a `Gap` cannot be the first chunk.
206+
if item_index == 0 && chunk.is_items() && chunk.previous.is_some() {
207+
let previous_chunk = chunk
208+
.previous_mut()
209+
// SAFETY: The `previous` chunk exists because we have tested
210+
// `chunk.previous.is_some()` in the `if` statement.
211+
.unwrap();
212+
213+
previous_chunk.insert_next(Chunk::new_gap_leaked(
214+
chunk_identifier_generator.generate_next().unwrap(),
215+
content,
216+
));
217+
218+
// We don't need to update `self.last` because we have inserted a new chunk
219+
// before `chunk`.
220+
221+
return Ok(());
222+
}
223+
200224
let chunk = match &mut chunk.content {
201225
ChunkContent::Gap(..) => {
202226
return Err(LinkedChunkError::ChunkIsAGap { identifier: chunk_identifier });
@@ -334,15 +358,15 @@ impl<Item, Gap, const CAP: usize> LinkedChunk<Item, Gap, CAP> {
334358
}
335359
}
336360

337-
/// Search for a chunk, and return its identifier.
361+
/// Search backward for a chunk, and return its identifier.
338362
pub fn chunk_identifier<'a, P>(&'a self, mut predicate: P) -> Option<ChunkIdentifier>
339363
where
340364
P: FnMut(&'a Chunk<Item, Gap, CAP>) -> bool,
341365
{
342366
self.rchunks().find_map(|chunk| predicate(chunk).then(|| chunk.identifier()))
343367
}
344368

345-
/// Search for an item, and return its position.
369+
/// Search backward for an item, and return its position.
346370
pub fn item_position<'a, P>(&'a self, mut predicate: P) -> Option<Position>
347371
where
348372
P: FnMut(&'a Item) -> bool,
@@ -941,75 +965,60 @@ mod tests {
941965
};
942966

943967
macro_rules! assert_items_eq {
944-
( @_ [ $iterator:ident, $chunk_index:ident, $item_index:ident ] { [-] $( $rest:tt )* } { $( $accumulator:tt )* } ) => {
968+
( @_ [ $iterator:ident ] { [-] $( $rest:tt )* } { $( $accumulator:tt )* } ) => {
945969
assert_items_eq!(
946970
@_
947-
[ $iterator, $chunk_index, $item_index ]
971+
[ $iterator ]
948972
{ $( $rest )* }
949973
{
950974
$( $accumulator )*
951-
$chunk_index += 1;
975+
{
976+
let chunk = $iterator .next().expect("next chunk (expect gap)");
977+
assert!(chunk.is_gap(), "chunk ");
978+
}
952979
}
953980
)
954981
};
955982

956-
( @_ [ $iterator:ident, $chunk_index:ident, $item_index:ident ] { [ $( $item:expr ),* ] $( $rest:tt )* } { $( $accumulator:tt )* } ) => {
983+
( @_ [ $iterator:ident ] { [ $( $item:expr ),* ] $( $rest:tt )* } { $( $accumulator:tt )* } ) => {
957984
assert_items_eq!(
958985
@_
959-
[ $iterator, $chunk_index, $item_index ]
986+
[ $iterator ]
960987
{ $( $rest )* }
961988
{
962989
$( $accumulator )*
963-
let _expected_chunk_identifier = $iterator .peek().unwrap().1.chunk_identifier();
964-
$(
965-
assert_matches!(
966-
$iterator .next(),
967-
Some((chunk_index, Position(chunk_identifier, item_index), & $item )) => {
968-
// Ensure the chunk index (from the enumeration) is correct.
969-
assert_eq!(chunk_index, $chunk_index);
970-
// Ensure the chunk identifier is the same for all items in this chunk.
971-
assert_eq!(chunk_identifier, _expected_chunk_identifier);
972-
// Ensure the item has the expected position.
973-
assert_eq!(item_index, $item_index);
974-
}
975-
);
976-
$item_index += 1;
977-
)*
978-
$item_index = 0;
979-
$chunk_index += 1;
990+
{
991+
let chunk = $iterator .next().expect("next chunk (expect items)");
992+
assert!(chunk.is_items());
993+
994+
let ChunkContent::Items(items) = chunk.content() else { unreachable!() };
995+
996+
let mut items_iterator = items.iter();
997+
998+
$(
999+
assert_eq!(items_iterator.next(), Some(& $item ));
1000+
)*
1001+
1002+
assert!(items_iterator.next().is_none(), "no more items");
1003+
}
9801004
}
9811005
)
9821006
};
9831007

984-
( @_ [ $iterator:ident, $chunk_index:ident, $item_index:ident ] {} { $( $accumulator:tt )* } ) => {
1008+
( @_ [ $iterator:ident ] {} { $( $accumulator:tt )* } ) => {
9851009
{
986-
let mut $chunk_index = 0;
987-
let mut $item_index = 0;
9881010
$( $accumulator )*
1011+
assert!( $iterator .next().is_none(), "no more chunks");
9891012
}
9901013
};
9911014

9921015
( $linked_chunk:expr, $( $all:tt )* ) => {
9931016
assert_items_eq!(
9941017
@_
995-
[ iterator, _chunk_index, _item_index ]
1018+
[ iterator ]
9961019
{ $( $all )* }
9971020
{
998-
let mut iterator = $linked_chunk
999-
.chunks()
1000-
.enumerate()
1001-
.filter_map(|(chunk_index, chunk)| match &chunk.content {
1002-
ChunkContent::Gap(..) => None,
1003-
ChunkContent::Items(items) => {
1004-
let identifier = chunk.identifier();
1005-
1006-
Some(items.iter().enumerate().map(move |(item_index, item)| {
1007-
(chunk_index, Position(identifier, item_index), item)
1008-
}))
1009-
}
1010-
})
1011-
.flatten()
1012-
.peekable();
1021+
let mut iterator = $linked_chunk.chunks();
10131022
}
10141023
)
10151024
}
@@ -1392,14 +1401,48 @@ mod tests {
13921401
assert_items_eq!(linked_chunk, ['a'] [-] ['b', 'c'] ['d', 'e', 'f']);
13931402
}
13941403

1395-
// Insert at the beginning of a chunk.
1404+
// Insert at the beginning of a chunk + it's the first chunk.
13961405
{
13971406
let position_of_a = linked_chunk.item_position(|item| *item == 'a').unwrap();
13981407
linked_chunk.insert_gap_at((), position_of_a)?;
13991408

1409+
// A new empty chunk is created as the first chunk.
14001410
assert_items_eq!(linked_chunk, [] [-] ['a'] [-] ['b', 'c'] ['d', 'e', 'f']);
14011411
}
14021412

1413+
// Insert at the beginning of a chunk.
1414+
{
1415+
let position_of_d = linked_chunk.item_position(|item| *item == 'd').unwrap();
1416+
linked_chunk.insert_gap_at((), position_of_d)?;
1417+
1418+
// A new empty chunk is NOT created, i.e. `['d', 'e', 'f']` is not
1419+
// split into `[]` + `['d', 'e', 'f']` because it's a waste of
1420+
// space.
1421+
assert_items_eq!(linked_chunk, [] [-] ['a'] [-] ['b', 'c'] [-] ['d', 'e', 'f']);
1422+
}
1423+
1424+
// Insert in an empty chunk + it's the first chunk.
1425+
{
1426+
let position_of_first_empty_chunk = Position(ChunkIdentifier(0), 0);
1427+
assert_matches!(
1428+
linked_chunk.insert_gap_at((), position_of_first_empty_chunk),
1429+
Err(LinkedChunkError::InvalidItemIndex { index: 0 })
1430+
);
1431+
}
1432+
1433+
// Insert in an empty chunk.
1434+
{
1435+
// Replace a gap by empty items.
1436+
let gap_identifier = linked_chunk.chunk_identifier(Chunk::is_gap).unwrap();
1437+
let position = linked_chunk.replace_gap_at([], gap_identifier)?.first_position();
1438+
1439+
assert_items_eq!(linked_chunk, [] [-] ['a'] [-] ['b', 'c'] [] ['d', 'e', 'f']);
1440+
1441+
linked_chunk.insert_gap_at((), position)?;
1442+
1443+
assert_items_eq!(linked_chunk, [] [-] ['a'] [-] ['b', 'c'] [-] [] ['d', 'e', 'f']);
1444+
}
1445+
14031446
// Insert in a chunk that does not exist.
14041447
{
14051448
assert_matches!(

0 commit comments

Comments
 (0)