Skip to content

Commit

Permalink
im-util: SortBy optimises Remove + Insert to Set in another case
Browse files Browse the repository at this point in the history
In the `SortBy` stream adapter, when the user is applying a
`observable_vector.set(…)` that should result in a `Remove` + `Insert`,
it sometimes can be optimised into a single `Set`. This happens when
the `old_index` and the `new_index` (resp. the sorted index of the
value to be updated, and the sorted index of the new value) are equal.
But actually, when `old_index` is lower than `new_index`, there is
_another_ missed optimisation in one particular context. If `old_index
== new_index - 1`, then we are actually updating the same location.
Indeed, the code is already making it clear: when `old_index <
new_index`, we need to subtract 1 to `new_index` because removing the
value at `old_index` is shifting all values at its right, thus 1 must
be subtracted to `new_index`. The missed opportunity for an optimisation
here is when `old_index == new_index - 1`, where it's clear that the
same position is updated. This patch handles this situation. The tests
are updated accordingly.
  • Loading branch information
Hywan authored Mar 14, 2024
1 parent 71e0a72 commit 9559347
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 5 deletions.
24 changes: 20 additions & 4 deletions eyeball-im-util/src/vector/sort.rs
Original file line number Diff line number Diff line change
Expand Up @@ -506,12 +506,28 @@ where
// Remove value at `old_index`, and insert the new value at `new_index - 1`: we need
// to subtract 1 because `old_index` has been removed before `new_insert`, which
// has shifted the indices.
//
// SAFETY: `new_index - 1` won't underflow because `new_index` is necessarily
// greater than `old_index` here. `old_index` cannot be lower than 0, so
// `new_index` cannot be lower than 1, hence `new_index - 1` cannot be lower
// than 0.
Ordering::Less => {
buffered_vector.remove(old_index);
buffered_vector.insert(new_index - 1, (new_unsorted_index, new_value.clone()));
let new_index = new_index - 1;
let new_unsorted_index_with_value = (new_unsorted_index, new_value.clone());

result.push(VectorDiff::Remove { index: old_index });
result.push(VectorDiff::Insert { index: new_index - 1, value: new_value });
// If `old_index == new_index`, we are clearly updating the same index.
// Then, let's emit a `VectorDiff::Set`.
if old_index == new_index {
buffered_vector.set(old_index, new_unsorted_index_with_value);

result.push(VectorDiff::Set { index: old_index, value: new_value });
} else {
buffered_vector.remove(old_index);
buffered_vector.insert(new_index, new_unsorted_index_with_value);

result.push(VectorDiff::Remove { index: old_index });
result.push(VectorDiff::Insert { index: new_index, value: new_value });
}
}
// `old_index` is the same as `new_index`.
Ordering::Equal => {
Expand Down
10 changes: 9 additions & 1 deletion eyeball-im-util/tests/it/sort.rs
Original file line number Diff line number Diff line change
Expand Up @@ -338,10 +338,18 @@ fn set() {
ob.set(0, 'd');
assert_next_eq!(sub, VectorDiff::Set { index: 1, value: 'd' });

// Another value, that is sorted at the same index.
// Another value, that is sorted at the same sorted index: `d` is at the sorted
// index 1, and `c` is at the sorted index 1 too. The `VectorDiff::Remove` +
// `VectorDiff::Insert` are optimised as a single `VectorDiff::Set`.
ob.set(0, 'c');
assert_next_eq!(sub, VectorDiff::Set { index: 1, value: 'c' });

// Another value, that is sorted at an adjacent sorted index: `c` is at the
// sorted index 1, but `d` is at the sorted index 2. The `VectorDiff::Remove` +
// `VectorDiff::Insert` are optimised as a single `VectorDiff::Set`.
ob.set(0, 'd');
assert_next_eq!(sub, VectorDiff::Set { index: 1, value: 'd' });

// Another value, that is moved to the left.
ob.set(0, 'a');
assert_next_eq!(sub, VectorDiff::Remove { index: 1 });
Expand Down

0 comments on commit 9559347

Please sign in to comment.