From 9559347cf3b56b9d341f5e8650f066a4071aa8e7 Mon Sep 17 00:00:00 2001
From: Ivan Enderlin <ivan@mnt.io>
Date: Thu, 14 Mar 2024 21:54:20 +0100
Subject: [PATCH] im-util: `SortBy` optimises `Remove` + `Insert` to `Set` in
 another case
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 eyeball-im-util/src/vector/sort.rs | 24 ++++++++++++++++++++----
 eyeball-im-util/tests/it/sort.rs   | 10 +++++++++-
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/eyeball-im-util/src/vector/sort.rs b/eyeball-im-util/src/vector/sort.rs
index bd315da..557225a 100644
--- a/eyeball-im-util/src/vector/sort.rs
+++ b/eyeball-im-util/src/vector/sort.rs
@@ -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 => {
diff --git a/eyeball-im-util/tests/it/sort.rs b/eyeball-im-util/tests/it/sort.rs
index 54642af..604c29c 100644
--- a/eyeball-im-util/tests/it/sort.rs
+++ b/eyeball-im-util/tests/it/sort.rs
@@ -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 });