status-im
/
realm-js
mirror of https://github.com/status-im/realm-js.git
2
0
Fork 0
Code Issues Projects Releases Wiki Activity

Discard moves which are turned into no-ops when merging

This commit is contained in:
Thomas Goyne 2016-03-10 16:58:43 -08:00
parent cfb9f0635c
commit e65ad4d413
7 changed files with 117 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

56
src/collection_notifications.cpp
View File

@ -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<RowInfo>& new_rows, CollectionChangeIndices& changeset,
std::function<bool (size_t)> row_did_change)
void calculate_moves_unsorted(std::vector<RowInfo>& 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<RowInfo>& 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<RowInfo>& new_rows, CollectionChangeIndices& changeset,
std::function<bool (size_t)> row_did_change)
void calculate_moves_sorted(std::vector<RowInfo>& new_rows, CollectionChangeIndices& changeset)
{
std::vector<std::pair<size_t, size_t>> old_candidates;
std::vector<std::pair<size_t, size_t>> 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<RowInfo>& 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<size_t> 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();

1
src/impl/background_collection.cpp
View File

@ -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();
}

2
src/impl/list_notifier.cpp
View File

@ -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()))

10
src/index_set.cpp
View File

@ -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)

59
tests/collection_change_indices.cpp
View File

@ -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<size_t> 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());
}
}
}

19
tests/results.cpp
View File

@ -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();

1
tests/util/index_helpers.hpp
View File

@ -2,6 +2,7 @@
index_set.verify(); \
std::initializer_list<size_t> 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) { \