Prioritize modified rows when calculating changes for sorted results

src/collection_notifications.cpp

@@ -357,10 +357,11 @@ void calculate_moves_unsorted(std::vector<RowInfo>& new_rows, CollectionChangeIn
 using items = std::vector<std::pair<size_t, size_t>>;
 
 struct Match {
-    size_t i, j, size;
+    size_t i, j, size, modified;
 };
 
 Match find_longest_match(items const& a, items const& b,
+                         IndexSet const& modified,
                          size_t begin1, size_t end1, size_t begin2, size_t end2)
 {
     Match best = {begin1, begin2, 0};
@@ -384,11 +385,18 @@ Match find_longest_match(items const& a, items const& b,
             size_t off = j - begin2;
             size_t size = off == 0 ? 1 : len_from_j_prev[off - 1] + 1;
             len_from_j[off] = size;
-            if (size > best.size) {
+            if (size > best.size)
-                best.i = i - size + 1;
+                best = {i - size + 1, j - size + 1, size, npos};
-                best.j = j - size + 1;
+            // Given two equal-length matches, prefer the one with fewer modified rows
-                best.size = size;
+            else if (size == best.size) {
+                if (best.modified == npos)
+                    best.modified = modified.count(best.j - size + 1, best.j + 1);
+                auto count = modified.count(j - size + 1, j + 1);
+                if (count < best.modified)
+                    best = {i - size + 1, j - size + 1, size, count};
             }
+            REALM_ASSERT(best.i >= begin1 && best.i + best.size <= end1);
+            REALM_ASSERT(best.j >= begin2 && best.j + best.size <= end2);
         }
         len_from_j.swap(len_from_j_prev);
     }
@@ -396,51 +404,40 @@ Match find_longest_match(items const& a, items const& b,
 }
 
 void find_longest_matches(items const& a, items const& b_ndx,
-                          size_t begin1, size_t end1, size_t begin2, size_t end2, std::vector<Match>& ret)
+                          size_t begin1, size_t end1, size_t begin2, size_t end2,
+                          IndexSet const& modified, std::vector<Match>& ret)
 {
     // FIXME: recursion could get too deep here
-    Match m = find_longest_match(a, b_ndx, begin1, end1, begin2, end2);
+    auto m = find_longest_match(a, b_ndx, modified, begin1, end1, begin2, end2);
     if (!m.size)
         return;
     if (m.i > begin1 && m.j > begin2)
-        find_longest_matches(a, b_ndx, begin1, m.i, begin2, m.j, ret);
+        find_longest_matches(a, b_ndx, begin1, m.i, begin2, m.j, modified, ret);
     ret.push_back(m);
     if (m.i + m.size < end2 && m.j + m.size < end2)
-        find_longest_matches(a, b_ndx, m.i + m.size, end1, m.j + m.size, end2, ret);
+        find_longest_matches(a, b_ndx, m.i + m.size, end1, m.j + m.size, end2, modified, ret);
 }
 
 void calculate_moves_sorted(std::vector<RowInfo>& new_rows, CollectionChangeIndices& changeset,
                             std::function<bool (size_t)> row_did_change)
 {
+    // Remove new insertions, as they're already reported in changeset
+    new_rows.erase(std::remove_if(begin(new_rows), end(new_rows),
+                                  [](auto& row) { return row.prev_tv_index == npos; }),
+                   end(new_rows));
+
+    std::sort(begin(new_rows), end(new_rows),
+              [](auto& lft, auto& rgt) { return lft.tv_index < rgt.tv_index; });
+
     std::vector<std::pair<size_t, size_t>> old_candidates;
     std::vector<std::pair<size_t, size_t>> new_candidates;
-
-    std::sort(begin(new_rows), end(new_rows), [](auto& lft, auto& rgt) {
-        return lft.tv_index < rgt.tv_index;
-    });
-
-    IndexSet::IndexInterator ins = changeset.insertions.begin(), del = changeset.deletions.begin();
-    int shift = 0;
     for (auto& row : new_rows) {
-        while (del != changeset.deletions.end() && *del <= row.tv_index) {
-            ++del;
-            ++shift;
-        }
-        while (ins != changeset.insertions.end() && *ins <= row.tv_index) {
-            ++ins;
-            --shift;
-        }
-        if (row.prev_tv_index == npos)
-            continue;
-
         if (row_did_change(row.shifted_row_index)) {
-            // FIXME: needlessly quadratic
-            if (!changeset.insertions.contains(row.tv_index))
             changeset.modifications.add(row.tv_index);
         }
+
         old_candidates.push_back({row.shifted_row_index, row.prev_tv_index});
         new_candidates.push_back({row.shifted_row_index, row.tv_index});
-//        }
     }
 
     std::sort(begin(old_candidates), end(old_candidates), [](auto a, auto b) {
@@ -477,7 +474,7 @@ void calculate_moves_sorted(std::vector<RowInfo>& new_rows, CollectionChangeIndi
     find_longest_matches(old_candidates, b_ndx,
                          first_difference, old_candidates.size(),
                          first_difference, new_candidates.size(),
-                         longest_matches);
+                         changeset.modifications, longest_matches);
     longest_matches.push_back({old_candidates.size(), new_candidates.size(), 0});
 
     size_t i = first_difference, j = first_difference;
@@ -553,5 +550,22 @@ CollectionChangeIndices CollectionChangeIndices::calculate(std::vector<size_t> c
     }
     ret.verify();
 
+#ifdef REALM_DEBUG
+    { // Verify that applying the calculated change to prev_rows actually produces next_rows
+        auto rows = prev_rows;
+        auto it = std::make_reverse_iterator(ret.deletions.end());
+        auto end = std::make_reverse_iterator(ret.deletions.begin());
+        for (; it != end; ++it) {
+            rows.erase(rows.begin() + it->first, rows.begin() + it->second);
+        }
+
+        for (auto i : ret.insertions.as_indexes()) {
+            rows.insert(rows.begin() + i, next_rows[i]);
+        }
+
+        REALM_ASSERT(rows == next_rows);
+    }
+#endif
+
     return ret;
 }

src/impl/async_query.cpp

@@ -133,6 +133,7 @@ void AsyncQuery::run()
         m_changes = CollectionChangeIndices::calculate(m_previous_rows, next_rows,
                                                        [&](size_t row) { return m_info->row_did_change(*m_query->get_table(), row); },
                                                        !!m_sort);
+
         m_previous_rows = std::move(next_rows);
         if (m_changes.empty()) {
             m_tv = {};

src/index_set.cpp

@@ -36,6 +36,17 @@ bool IndexSet::contains(size_t index) const
     return it != m_ranges.end() && it->first <= index;
 }
 
+size_t IndexSet::count(size_t start_index, size_t end_index) const
+{
+    auto it = const_cast<IndexSet*>(this)->find(start_index);
+    size_t ret = 0;
+    for (; end_index > start_index && it != m_ranges.end() && it->first < end_index; ++it) {
+        ret += std::min(it->second, end_index) - std::max(it->first, start_index);
+        start_index = it->second;
+    }
+    return ret;
+}
+
 IndexSet::iterator IndexSet::find(size_t index)
 {
     return find(index, m_ranges.begin());

src/index_set.hpp

@@ -44,6 +44,9 @@ public:
     // Check if the index set contains the given index
     bool contains(size_t index) const;
 
+    // Counts the number of indices in the set in the given range
+    size_t count(size_t start_index, size_t end_index) const;
+
     // Add an index to the set, doing nothing if it's already present
     void add(size_t index);
     void add(IndexSet const& is);

tests/collection_change_indices.cpp

@@ -242,6 +242,29 @@ TEST_CASE("collection change indices") {
             REQUIRE_MOVES(calc({3, 1, 2}), {2, 0});
             REQUIRE_MOVES(calc({3, 2, 1}), {2, 0}, {1, 1});
         }
+
+        SECTION("merge can collapse insert -> move -> delete to no-op") {
+            auto four_modified = [](size_t ndx) { return ndx == 4; };
+            for (int insert_pos = 0; insert_pos < 4; ++insert_pos) {
+                for (int move_to_pos = 0; move_to_pos < 4; ++move_to_pos) {
+                    if (insert_pos == move_to_pos)
+                        continue;
+                    CAPTURE(insert_pos);
+                    CAPTURE(move_to_pos);
+
+                    std::vector<size_t> after_insert = {1, 2, 3};
+                    after_insert.insert(after_insert.begin() + insert_pos, 4);
+                    c = CollectionChangeIndices::calculate({1, 2, 3}, after_insert, four_modified, true);
+
+                    std::vector<size_t> after_move = {1, 2, 3};
+                    after_move.insert(after_move.begin() + move_to_pos, 4);
+                    c.merge(CollectionChangeIndices::calculate(after_insert, after_move, four_modified, true));
+
+                    c.merge(CollectionChangeIndices::calculate(after_move, {1, 2, 3}, four_modified, true));
+                    REQUIRE(c.empty());
+                }
+            }
+        }
     }
 
     SECTION("merge") {

tests/index_set.cpp

@@ -7,6 +7,36 @@
 TEST_CASE("index set") {
     realm::IndexSet set;
 
+    SECTION("contains() returns if the index is in the set") {
+        set = {1, 2, 3, 5};
+        REQUIRE_FALSE(set.contains(0));
+        REQUIRE(set.contains(1));
+        REQUIRE(set.contains(2));
+        REQUIRE(set.contains(3));
+        REQUIRE_FALSE(set.contains(4));
+        REQUIRE(set.contains(5));
+        REQUIRE_FALSE(set.contains(6));
+    }
+
+    SECTION("count() returns the number of indices int he range in the set") {
+        set = {1, 2, 3, 5};
+        REQUIRE(set.count(0, 6) == 4);
+        REQUIRE(set.count(0, 5) == 3);
+        REQUIRE(set.count(0, 4) == 3);
+        REQUIRE(set.count(0, 3) == 2);
+        REQUIRE(set.count(0, 2) == 1);
+        REQUIRE(set.count(0, 1) == 0);
+        REQUIRE(set.count(0, 0) == 0);
+
+        REQUIRE(set.count(0, 6) == 4);
+        REQUIRE(set.count(1, 6) == 4);
+        REQUIRE(set.count(2, 6) == 3);
+        REQUIRE(set.count(3, 6) == 2);
+        REQUIRE(set.count(4, 6) == 1);
+        REQUIRE(set.count(5, 6) == 1);
+        REQUIRE(set.count(6, 6) == 0);
+    }
+
     SECTION("add() extends existing ranges") {
         set.add(1);
         REQUIRE_INDICES(set, 1);