Skip to content

Commit

Permalink
feat(sdk): Optimise how insert_gap_at works when inserting at first…
Browse files Browse the repository at this point in the history 
… 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.
  • Loading branch information
@Hywan
Hywan committed Mar 20, 2024
1 parent df4dc04 commit 6222bbd
Showing 1 changed file with 87 additions and 44 deletions.
131 changes: 87 additions & 44 deletions crates/matrix-sdk/src/event_cache/linked_chunk.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,30 @@ impl<Item, Gap, const CAP: usize> LinkedChunk<Item, Gap, CAP> {
.chunk_mut(chunk_identifier)
.ok_or(LinkedChunkError::InvalidChunkIdentifier { identifier: chunk_identifier })?;

// If `item_index` is 0, we don't want to split the current items chunk to
// insert a new gap chunk, otherwise it would create an empty current items
// chunk. Let's handle this case in particular.
//
// Of course this optimisation applies if there is a previous chunk. Remember
// the invariant: a `Gap` cannot be the first chunk.
if item_index == 0 && chunk.is_items() && chunk.previous.is_some() {
let previous_chunk = chunk
.previous_mut()
// SAFETY: The `previous` chunk exists because we have tested
// `chunk.previous.is_some()` in the `if` statement.
.unwrap();

previous_chunk.insert_next(Chunk::new_gap_leaked(
chunk_identifier_generator.generate_next().unwrap(),
content,
));

// We don't need to update `self.last` because we have inserted a new chunk
// before `chunk`.

return Ok(());
}

let chunk = match &mut chunk.content {
ChunkContent::Gap(..) => {
return Err(LinkedChunkError::ChunkIsAGap { identifier: chunk_identifier });
Expand Down Expand Up @@ -345,15 +369,15 @@ impl<Item, Gap, const CAP: usize> LinkedChunk<Item, Gap, CAP> {
}
}

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

/// Search for an item, and return its position.
/// Search backward for an item, and return its position.
pub fn item_position<'a, P>(&'a self, mut predicate: P) -> Option<Position>
where
P: FnMut(&'a Item) -> bool,
Expand Down Expand Up @@ -952,75 +976,60 @@ mod tests {
};

macro_rules! assert_items_eq {
( @_ [ $iterator:ident, $chunk_index:ident, $item_index:ident ] { [-] $( $rest:tt )* } { $( $accumulator:tt )* } ) => {
( @_ [ $iterator:ident ] { [-] $( $rest:tt )* } { $( $accumulator:tt )* } ) => {
assert_items_eq!(
@_
[ $iterator, $chunk_index, $item_index ]
[ $iterator ]
{ $( $rest )* }
{
$( $accumulator )*
$chunk_index += 1;
{
let chunk = $iterator .next().expect("next chunk (expect gap)");
assert!(chunk.is_gap(), "chunk ");
}
}
)
};

( @_ [ $iterator:ident, $chunk_index:ident, $item_index:ident ] { [ $( $item:expr ),* ] $( $rest:tt )* } { $( $accumulator:tt )* } ) => {
( @_ [ $iterator:ident ] { [ $( $item:expr ),* ] $( $rest:tt )* } { $( $accumulator:tt )* } ) => {
assert_items_eq!(
@_
[ $iterator, $chunk_index, $item_index ]
[ $iterator ]
{ $( $rest )* }
{
$( $accumulator )*
let _expected_chunk_identifier = $iterator .peek().unwrap().1.chunk_identifier();
$(
assert_matches!(
$iterator .next(),
Some((chunk_index, Position(chunk_identifier, item_index), & $item )) => {
// Ensure the chunk index (from the enumeration) is correct.
assert_eq!(chunk_index, $chunk_index);
// Ensure the chunk identifier is the same for all items in this chunk.
assert_eq!(chunk_identifier, _expected_chunk_identifier);
// Ensure the item has the expected position.
assert_eq!(item_index, $item_index);
}
);
$item_index += 1;
)*
$item_index = 0;
$chunk_index += 1;
{
let chunk = $iterator .next().expect("next chunk (expect items)");
assert!(chunk.is_items());

let ChunkContent::Items(items) = chunk.content() else { unreachable!() };

let mut items_iterator = items.iter();

$(
assert_eq!(items_iterator.next(), Some(& $item ));
)*

assert!(items_iterator.next().is_none(), "no more items");
}
}
)
};

( @_ [ $iterator:ident, $chunk_index:ident, $item_index:ident ] {} { $( $accumulator:tt )* } ) => {
( @_ [ $iterator:ident ] {} { $( $accumulator:tt )* } ) => {
{
let mut $chunk_index = 0;
let mut $item_index = 0;
$( $accumulator )*
assert!( $iterator .next().is_none(), "no more chunks");
}
};

( $linked_chunk:expr, $( $all:tt )* ) => {
assert_items_eq!(
@_
[ iterator, _chunk_index, _item_index ]
[ iterator ]
{ $( $all )* }
{
let mut iterator = $linked_chunk
.chunks()
.enumerate()
.filter_map(|(chunk_index, chunk)| match &chunk.content {
ChunkContent::Gap(..) => None,
ChunkContent::Items(items) => {
let identifier = chunk.identifier();

Some(items.iter().enumerate().map(move |(item_index, item)| {
(chunk_index, Position(identifier, item_index), item)
}))
}
})
.flatten()
.peekable();
let mut iterator = $linked_chunk.chunks();
}
)
}
Expand Down Expand Up @@ -1403,14 +1412,48 @@ mod tests {
assert_items_eq!(linked_chunk, ['a'] [-] ['b', 'c'] ['d', 'e', 'f']);
}

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

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

// Insert at the beginning of a chunk.
{
let position_of_d = linked_chunk.item_position(|item| *item == 'd').unwrap();
linked_chunk.insert_gap_at((), position_of_d)?;

// A new empty chunk is NOT created, i.e. `['d', 'e', 'f']` is not
// split into `[]` + `['d', 'e', 'f']` because it's a waste of
// space.
assert_items_eq!(linked_chunk, [] [-] ['a'] [-] ['b', 'c'] [-] ['d', 'e', 'f']);
}

// Insert in an empty chunk + it's the first chunk.
{
let position_of_first_empty_chunk = Position(ChunkIdentifier(0), 0);
assert_matches!(
linked_chunk.insert_gap_at((), position_of_first_empty_chunk),
Err(LinkedChunkError::InvalidItemIndex { index: 0 })
);
}

// Insert in an empty chunk.
{
// Replace a gap by empty items.
let gap_identifier = linked_chunk.chunk_identifier(Chunk::is_gap).unwrap();
let position = linked_chunk.replace_gap_at([], gap_identifier)?.first_position();

assert_items_eq!(linked_chunk, [] [-] ['a'] [-] ['b', 'c'] [] ['d', 'e', 'f']);

linked_chunk.insert_gap_at((), position)?;

assert_items_eq!(linked_chunk, [] [-] ['a'] [-] ['b', 'c'] [-] [] ['d', 'e', 'f']);
}

// Insert in a chunk that does not exist.
{
assert_matches!(
Expand Down

0 comments on commit 6222bbd

Please sign in to comment.