From e65ad4d413ddecc30bd0f6b745c28a48f0732343 Mon Sep 17 00:00:00 2001 From: Thomas Goyne Date: Thu, 10 Mar 2016 16:58:43 -0800 Subject: [PATCH] Discard moves which are turned into no-ops when merging --- src/collection_notifications.cpp | 56 ++++++++++++++++----------- src/impl/background_collection.cpp | 1 + src/impl/list_notifier.cpp | 2 +- src/index_set.cpp | 10 +++++ tests/collection_change_indices.cpp | 59 +++++++++++++++++++++++++---- tests/results.cpp | 19 ++++++++++ tests/util/index_helpers.hpp | 1 + 7 files changed, 117 insertions(+), 31 deletions(-) diff --git a/src/collection_notifications.cpp b/src/collection_notifications.cpp index 6eef867e..ee0506cc 100644 --- a/src/collection_notifications.cpp +++ b/src/collection_notifications.cpp @@ -102,6 +102,7 @@ void CollectionChangeBuilder::merge(CollectionChangeBuilder&& c) } // Check if the destination was deleted + // Removing the insert for this move will happen later if (c.deletions.contains(old.to)) return true; @@ -114,12 +115,20 @@ void CollectionChangeBuilder::merge(CollectionChangeBuilder&& c) // Ignore new moves of rows which were previously inserted (the implicit // delete from the move will remove the insert) - if (!insertions.empty()) { + if (!insertions.empty() && !c.moves.empty()) { c.moves.erase(remove_if(begin(c.moves), end(c.moves), [&](auto const& m) { return insertions.contains(m.from); }), end(c.moves)); } + // Ensure that any previously modified rows which were moved are still modified + if (!modifications.empty() && !c.moves.empty()) { + for (auto const& move : c.moves) { + if (modifications.contains(move.from)) + c.modifications.add(move.to); + } + } + // Update the source position of new moves to compensate for the changes made // in the old changeset if (!deletions.empty() || !insertions.empty()) { @@ -137,8 +146,17 @@ void CollectionChangeBuilder::merge(CollectionChangeBuilder&& c) insertions.erase_at(c.deletions); insertions.insert_at(c.insertions); - // Ignore new mmodifications to previously inserted rows - c.modifications.remove(insertions); + // Look for moves which are now no-ops, and remove them plus the associated + // insert+delete. Note that this isn't just checking for from == to due to + // that rows can also be shifted by other inserts and deletes + IndexSet to_remove; + moves.erase(remove_if(begin(moves), end(moves), [&](auto const& move) { + if (move.from - deletions.count(0, move.from) != move.to - insertions.count(0, move.to)) + return false; + deletions.remove(move.from); + insertions.remove(move.to); + return true; + }), end(moves)); modifications.erase_at(c.deletions); modifications.shift_for_insert_at(c.insertions); @@ -297,10 +315,6 @@ void CollectionChangeBuilder::verify() REALM_ASSERT(deletions.contains(move.from)); REALM_ASSERT(insertions.contains(move.to)); } -// for (auto index : modifications.as_indexes()) -// REALM_ASSERT(!insertions.contains(index)); -// for (auto index : insertions.as_indexes()) -// REALM_ASSERT(!modifications.contains(index)); #endif } @@ -311,8 +325,7 @@ struct RowInfo { size_t tv_index; }; -void calculate_moves_unsorted(std::vector& new_rows, CollectionChangeIndices& changeset, - std::function row_did_change) +void calculate_moves_unsorted(std::vector& new_rows, CollectionChangeIndices& changeset) { for (auto& row : new_rows) { // Calculate where this row would be with only previous insertions @@ -326,9 +339,6 @@ void calculate_moves_unsorted(std::vector& new_rows, CollectionChangeIn changeset.insertions.add(row.tv_index); changeset.deletions.add(row.prev_tv_index); } - else if (!changeset.insertions.contains(row.tv_index) && row_did_change(row.shifted_row_index)) { - changeset.modifications.add(row.tv_index); - } } } @@ -396,16 +406,11 @@ void find_longest_matches(items const& a, items const& b_ndx, 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) +void calculate_moves_sorted(std::vector& new_rows, CollectionChangeIndices& changeset) { std::vector> old_candidates; std::vector> new_candidates; for (auto& row : new_rows) { - if (row_did_change(row.shifted_row_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}); } @@ -456,9 +461,6 @@ void calculate_moves_sorted(std::vector& new_rows, CollectionChangeIndi i += match.size; j += match.size; } - - // FIXME: needlessly suboptimal - changeset.modifications.remove(changeset.insertions); } } // Anonymous namespace @@ -512,17 +514,25 @@ CollectionChangeBuilder CollectionChangeBuilder::calculate(std::vector c for (; j < new_rows.size(); ++j) ret.insertions.add(new_rows[j].tv_index); + // Filter out the new insertions since we don't need them for any of the + // further calculations 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; }); + for (auto& row : new_rows) { + if (row_did_change(row.shifted_row_index)) { + ret.modifications.add(row.tv_index); + } + } + if (sort) { - calculate_moves_sorted(new_rows, ret, row_did_change); + calculate_moves_sorted(new_rows, ret); } else { - calculate_moves_unsorted(new_rows, ret, row_did_change); + calculate_moves_unsorted(new_rows, ret); } ret.verify(); diff --git a/src/impl/background_collection.cpp b/src/impl/background_collection.cpp index e7b1c753..5ce39b45 100644 --- a/src/impl/background_collection.cpp +++ b/src/impl/background_collection.cpp @@ -200,6 +200,7 @@ bool BackgroundCollection::deliver(SharedGroup& sg, std::exception_ptr err) bool should_call_callbacks = do_deliver(sg); m_changes_to_deliver = std::move(m_accumulated_changes); + m_changes_to_deliver.modifications.remove(m_changes_to_deliver.insertions); return should_call_callbacks && have_callbacks(); } diff --git a/src/impl/list_notifier.cpp b/src/impl/list_notifier.cpp index ef451917..e8291fad 100644 --- a/src/impl/list_notifier.cpp +++ b/src/impl/list_notifier.cpp @@ -98,9 +98,9 @@ void ListNotifier::run() m_change.moves.erase(remove_if(begin(m_change.moves), end(m_change.moves), [&](auto move) { return m_change.modifications.contains(move.to); }), end(m_change.moves)); - m_change.modifications.remove(m_change.insertions); for (size_t i = 0; i < m_lv->size(); ++i) { + // FIXME: may need to mark modifications even for inserts (for moves?) if (m_change.insertions.contains(i) || m_change.modifications.contains(i)) continue; if (m_info->row_did_change(m_lv->get_target_table(), m_lv->get(i).get_index())) diff --git a/src/index_set.cpp b/src/index_set.cpp index 0df1b787..3468f439 100644 --- a/src/index_set.cpp +++ b/src/index_set.cpp @@ -86,6 +86,14 @@ size_t IndexSet::add_shifted(size_t index) void IndexSet::add_shifted_by(IndexSet const& shifted_by, IndexSet const& values) { +#ifdef REALM_DEBUG + size_t expected = std::distance(as_indexes().begin(), as_indexes().end()); + for (auto index : values.as_indexes()) { + if (!shifted_by.contains(index)) + ++expected; + } +#endif + auto it = shifted_by.begin(), end = shifted_by.end(); size_t shift = 0; size_t skip_until = 0; @@ -100,6 +108,8 @@ void IndexSet::add_shifted_by(IndexSet const& shifted_by, IndexSet const& values ++shift; } } + + REALM_ASSERT_DEBUG(std::distance(as_indexes().begin(), as_indexes().end()) == expected); } void IndexSet::set(size_t len) diff --git a/tests/collection_change_indices.cpp b/tests/collection_change_indices.cpp index 0b06f3b0..327b5f83 100644 --- a/tests/collection_change_indices.cpp +++ b/tests/collection_change_indices.cpp @@ -8,7 +8,7 @@ TEST_CASE("collection change indices") { using namespace realm; _impl::CollectionChangeBuilder c; - SECTION("stuff") { + SECTION("basic mutation functions") { SECTION("insert() adds the row to the insertions set") { c.insert(5); c.insert(8); @@ -220,6 +220,12 @@ TEST_CASE("collection change indices") { REQUIRE(c.moves.empty()); } + SECTION("modifications are reported for moved rows") { + c = _impl::CollectionChangeBuilder::calculate({3, 5}, {5, 3}, all_modified, false); + REQUIRE_MOVES(c, {1, 0}); + REQUIRE_INDICES(c.modifications, 0, 1); + } + SECTION("unsorted reordering") { auto calc = [&](std::vector values) { return _impl::CollectionChangeBuilder::calculate({1, 2, 3}, values, none_modified, false); @@ -372,11 +378,11 @@ TEST_CASE("collection change indices") { REQUIRE_INDICES(c.modifications, 2); } - SECTION("modifications are discarded for previous insertions") { + SECTION("modifications are not discarded for previous insertions") { c = {{}, {2}, {}, {}}; c.merge({{}, {}, {1, 2, 3}}); REQUIRE_INDICES(c.insertions, 2); - REQUIRE_INDICES(c.modifications, 1, 3); + REQUIRE_INDICES(c.modifications, 1, 2, 3); } SECTION("modifications are merged with previous modifications") { @@ -385,10 +391,10 @@ TEST_CASE("collection change indices") { REQUIRE_INDICES(c.modifications, 1, 2, 3); } - SECTION("modifications are discarded for the destination of previous moves") { + SECTION("modifications are tracked for the destination of previous moves") { c = {{}, {}, {}, {{1, 2}}}; c.merge({{}, {}, {2, 3}}); - REQUIRE_INDICES(c.modifications, 3); + REQUIRE_INDICES(c.modifications, 2, 3); } SECTION("move sources are shifted for previous deletes and insertions") { @@ -405,10 +411,10 @@ TEST_CASE("collection change indices") { REQUIRE_MOVES(c, {5, 10}); } - SECTION("moves remove previous modifications to source") { + SECTION("moves update previous modifications to source") { c = {{}, {}, {1}, {}}; c.merge({{}, {}, {}, {{1, 3}}}); - REQUIRE(c.modifications.empty()); + REQUIRE_INDICES(c.modifications, 3); REQUIRE_MOVES(c, {1, 3}); } @@ -460,5 +466,44 @@ TEST_CASE("collection change indices") { c.merge({{}, {}, {}, {{6, 2}}}); REQUIRE_MOVES(c, {7, 2}); } + + SECTION("leapfrogging rows collapse to an empty changeset") { + c = {{1}, {0}, {}, {{1, 0}}}; + c.merge({{1}, {0}, {}, {{1, 0}}}); + REQUIRE(c.empty()); + } + + SECTION("modify -> move -> unmove leaves row marked as modified") { + c = {{}, {}, {1}}; + c.merge({{1}, {2}, {}, {{1, 2}}}); + c.merge({{1}}); + + REQUIRE_INDICES(c.deletions, 2); + REQUIRE(c.insertions.empty()); + REQUIRE(c.moves.empty()); + REQUIRE_INDICES(c.modifications, 1); + } + + SECTION("modifying a previously moved row which stops being a move due to more deletions") { + // Make it stop being a move in the same transaction as the modify + c = {{1, 2}, {0, 1}, {}, {{1, 0}, {2, 1}}}; + c.merge({{0, 2}, {1}, {0}, {}}); + + REQUIRE_INDICES(c.deletions, 0, 1); + REQUIRE_INDICES(c.insertions, 1); + REQUIRE_INDICES(c.modifications, 0); + REQUIRE(c.moves.empty()); + + // Same net change, but make it no longer a move in the transaction after the modify + c = {{1, 2}, {0, 1}, {}, {{1, 0}, {2, 1}}}; + c.merge({{}, {}, {1}, {}}); + c.merge({{0, 2}, {0}, {}, {{2, 0}}}); + c.merge({{0}, {1}, {}, {}}); + + REQUIRE_INDICES(c.deletions, 0, 1); + REQUIRE_INDICES(c.insertions, 1); + REQUIRE_INDICES(c.modifications, 0); + REQUIRE(c.moves.empty()); + } } } diff --git a/tests/results.cpp b/tests/results.cpp index 15c3ad71..ccf99890 100644 --- a/tests/results.cpp +++ b/tests/results.cpp @@ -247,6 +247,25 @@ TEST_CASE("Results") { REQUIRE(notification_calls == 2); } + SECTION("inserting a row then modifying it in a second transaction does not report it as modified") { + r->begin_transaction(); + size_t ndx = table->add_empty_row(); + table->set_int(0, ndx, 6); + r->commit_transaction(); + + coordinator->on_change(); + + r->begin_transaction(); + table->set_int(0, ndx, 7); + r->commit_transaction(); + + advance_and_notify(*r); + + REQUIRE(notification_calls == 2); + REQUIRE_INDICES(change.insertions, 4); + REQUIRE(change.modifications.empty()); + } + SECTION("notifications are not delivered when collapsing transactions results in no net change") { r->begin_transaction(); size_t ndx = table->add_empty_row(); diff --git a/tests/util/index_helpers.hpp b/tests/util/index_helpers.hpp index 4d847574..4608932d 100644 --- a/tests/util/index_helpers.hpp +++ b/tests/util/index_helpers.hpp @@ -2,6 +2,7 @@ index_set.verify(); \ std::initializer_list expected = {__VA_ARGS__}; \ auto actual = index_set.as_indexes(); \ + INFO("Checking " #index_set); \ REQUIRE(expected.size() == std::distance(actual.begin(), actual.end())); \ auto begin = actual.begin(), end = actual.end(); \ for (auto index : expected) { \