From 424f4e829f43106cf9e4901f21f7b9583d2faa40 Mon Sep 17 00:00:00 2001 From: Thomas Goyne Date: Tue, 8 Mar 2016 19:00:31 -0800 Subject: [PATCH] Prioritize modified rows when calculating changes for sorted results --- src/collection_notifications.cpp | 82 +++++++++++++++++------------ src/impl/async_query.cpp | 1 + src/index_set.cpp | 11 ++++ src/index_set.hpp | 3 ++ tests/collection_change_indices.cpp | 23 ++++++++ tests/index_set.cpp | 30 +++++++++++ 6 files changed, 116 insertions(+), 34 deletions(-) diff --git a/src/collection_notifications.cpp b/src/collection_notifications.cpp index fa13697e..262903b1 100644 --- a/src/collection_notifications.cpp +++ b/src/collection_notifications.cpp @@ -357,10 +357,11 @@ void calculate_moves_unsorted(std::vector& new_rows, CollectionChangeIn using items = std::vector>; 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) { - best.i = i - size + 1; - best.j = j - size + 1; - best.size = size; + if (size > best.size) + best = {i - size + 1, j - size + 1, size, npos}; + // Given two equal-length matches, prefer the one with fewer modified rows + 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& ret) + size_t begin1, size_t end1, size_t begin2, size_t end2, + IndexSet const& modified, std::vector& 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& new_rows, CollectionChangeIndices& changeset, std::function 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> old_candidates; std::vector> 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); + 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}); -// } + + 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& 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 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; } diff --git a/src/impl/async_query.cpp b/src/impl/async_query.cpp index f750a852..d77c2524 100644 --- a/src/impl/async_query.cpp +++ b/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 = {}; diff --git a/src/index_set.cpp b/src/index_set.cpp index ce1ad5b2..0df1b787 100644 --- a/src/index_set.cpp +++ b/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(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()); diff --git a/src/index_set.hpp b/src/index_set.hpp index 13748e8f..3186b5b3 100644 --- a/src/index_set.hpp +++ b/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); diff --git a/tests/collection_change_indices.cpp b/tests/collection_change_indices.cpp index e30f0593..74ef18aa 100644 --- a/tests/collection_change_indices.cpp +++ b/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 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 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") { diff --git a/tests/index_set.cpp b/tests/index_set.cpp index b7f58c6b..30737fb1 100644 --- a/tests/index_set.cpp +++ b/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);