diff --git a/CHANGELOG.md b/CHANGELOG.md index d588149b9f4..19e689e19d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ### Enhancements * (PR [#????](https://github.com/realm/realm-core/pull/????)) -* None. +* Improve performance of rolling back write transactions after making changes. If no KVO observers are used this is now constant time rather than taking time proportional to the number of changes to be rolled back. Rollbacks with KVO observers are 10-20% faster. ([PR #6513](https://github.com/realm/realm-core/pull/6513)) ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) @@ -18,6 +18,8 @@ ### Internals * Clear out SubscriptionStore and cancel pending notifications upon rollback to PBS after client migration to FLX. ([#6389](https://github.com/realm/realm-core/issues/6389)) +* Simplify the non-sync replication log by emitting the same instruction type for all three types of collections rather than different instructions per collection type. This has no functional effect but eliminates some duplicated code. ([PR #6513](https://github.com/realm/realm-core/pull/6513)) +* Remove TransactionChangeInfo::track_all, which was only ever used by the global notifier. ([PR #6513](https://github.com/realm/realm-core/pull/6513)) ---------------------------------------------- diff --git a/src/realm/exec/realm_trawler.cpp b/src/realm/exec/realm_trawler.cpp index e08b5b285b1..d19eeab9874 100644 --- a/src/realm/exec/realm_trawler.cpp +++ b/src/realm/exec/realm_trawler.cpp @@ -972,33 +972,15 @@ class HistoryLogger { return true; } - bool list_set(size_t ndx) + bool collection_set(size_t ndx) { - std::cout << "List set at " << ndx << std::endl; + std::cout << "Collection set at " << ndx << std::endl; return true; } - bool list_insert(size_t ndx) + bool collection_insert(size_t ndx) { - std::cout << "List insert at " << ndx << std::endl; - return true; - } - - bool dictionary_insert(size_t, realm::Mixed key) - { - std::cout << "Dictionary insert at " << key << std::endl; - return true; - } - - bool dictionary_set(size_t, realm::Mixed key) - { - std::cout << "Dictionary set at " << key << std::endl; - return true; - } - - bool dictionary_erase(size_t, realm::Mixed key) - { - std::cout << "Dictionary erase at " << key << std::endl; + std::cout << "Collection insert at " << ndx << std::endl; return true; } @@ -1029,39 +1011,21 @@ class HistoryLogger { return true; } - bool list_move(size_t from_link_ndx, size_t to_link_ndx) + bool collection_move(size_t from_link_ndx, size_t to_link_ndx) { std::cout << "List move from " << from_link_ndx << " to " << to_link_ndx << std::endl; return true; } - bool list_erase(size_t ndx) - { - std::cout << "List erase at " << ndx << std::endl; - return true; - } - - bool list_clear(size_t old_list_size) - { - std::cout << "List clear. Old size: " << old_list_size << std::endl; - return true; - } - - bool set_insert(size_t ndx) - { - std::cout << "Set insert at " << ndx << std::endl; - return true; - } - - bool set_erase(size_t ndx) + bool collection_erase(size_t ndx) { - std::cout << "Set erase at " << ndx << std::endl; + std::cout << "Collection erase at " << ndx << std::endl; return true; } - bool set_clear(size_t old_set_size) + bool collection_clear(size_t old_list_size) { - std::cout << "Set clear. Old size: " << old_set_size << std::endl; + std::cout << "Collection clear. Old size: " << old_list_size << std::endl; return true; } diff --git a/src/realm/group.cpp b/src/realm/group.cpp index 8383b932a81..f17db6a62bc 100644 --- a/src/realm/group.cpp +++ b/src/realm/group.cpp @@ -833,29 +833,23 @@ void Group::remove_table(size_t table_ndx, TableKey key) // columns of other tables, however, to do that, we would have to // automatically remove the "offending" link columns from those other // tables. Such a behaviour is deemed too obscure, and we shall therefore - // require that a removed table does not contain foreigh origin backlink + // require that a removed table does not contain foreign origin backlink // columns. if (table->is_cross_table_link_target()) throw CrossTableLinkTarget(table->get_name()); - // There is no easy way for Group::TransactAdvancer to handle removal of - // tables that contain foreign target table link columns, because that - // involves removal of the corresponding backlink columns. For that reason, - // we start by removing all columns, which will generate individual - // replication instructions for each column removal with sufficient - // information for Group::TransactAdvancer to handle them. - size_t n = table->get_column_count(); - Replication* repl = *get_repl(); - if (repl) { - // This will prevent sync instructions for column removals to be generated - repl->prepare_erase_class(key); - } - for (size_t i = n; i > 0; --i) { - ColKey col_key = table->spec_ndx2colkey(i - 1); - table->remove_column(col_key); + { + // We don't want to replicate the individual column removals along the + // way as they're covered by the table removal + Table::DisableReplication dr(*table); + for (size_t i = table->get_column_count(); i > 0; --i) { + ColKey col_key = table->spec_ndx2colkey(i - 1); + table->remove_column(col_key); + } } size_t prior_num_tables = m_tables.size(); + Replication* repl = *get_repl(); if (repl) repl->erase_class(key, prior_num_tables); // Throws @@ -1323,12 +1317,12 @@ class Group::TransactAdvancer { return true; // No-op } - bool list_set(size_t) + bool collection_set(size_t) { return true; } - bool list_insert(size_t) + bool collection_insert(size_t) { return true; } @@ -1366,46 +1360,21 @@ class Group::TransactAdvancer { return true; // No-op } - bool list_move(size_t, size_t) noexcept + bool collection_move(size_t, size_t) noexcept { return true; // No-op } - bool list_erase(size_t) noexcept + bool collection_erase(size_t) noexcept { return true; // No-op } - bool list_clear(size_t) noexcept - { - return true; // No-op - } - - bool dictionary_insert(size_t, Mixed) - { - return true; // No-op - } - bool dictionary_set(size_t, Mixed) - { - return true; // No-op - } - bool dictionary_erase(size_t, Mixed) + bool collection_clear(size_t) noexcept { return true; // No-op } - bool set_insert(size_t) - { - return true; // No-op - } - bool set_erase(size_t) - { - return true; // No-op - } - bool set_clear(size_t) - { - return true; // No-op - } bool typed_link_change(ColKey, TableKey) { return true; // No-op @@ -1476,7 +1445,7 @@ void Group::refresh_dirty_accessors() } -void Group::advance_transact(ref_type new_top_ref, util::NoCopyInputStream& in, bool writable) +void Group::advance_transact(ref_type new_top_ref, util::NoCopyInputStream* in, bool writable) { REALM_ASSERT(is_attached()); // Exception safety: If this function throws, the group accessor and all of @@ -1513,10 +1482,10 @@ void Group::advance_transact(ref_type new_top_ref, util::NoCopyInputStream& in, // This is no longer needed in Core, but we need to compute "schema_changed", // for the benefit of ObjectStore. bool schema_changed = false; - if (has_schema_change_notification_handler()) { - _impl::TransactLogParser parser; // Throws + if (in && has_schema_change_notification_handler()) { TransactAdvancer advancer(*this, schema_changed); - parser.parse(in, advancer); // Throws + _impl::TransactLogParser parser; // Throws + parser.parse(*in, advancer); // Throws } m_top.detach(); // Soft detach diff --git a/src/realm/group.hpp b/src/realm/group.hpp index a174fe38971..0dcea862fb1 100644 --- a/src/realm/group.hpp +++ b/src/realm/group.hpp @@ -696,7 +696,7 @@ class Group : public ArrayParent { class TransactAdvancer; /// Memory mappings must have been updated to reflect any growth in filesize before /// calling advance_transact() - void advance_transact(ref_type new_top_ref, util::NoCopyInputStream&, bool writable); + void advance_transact(ref_type new_top_ref, util::NoCopyInputStream*, bool writable); void refresh_dirty_accessors(); void flush_accessors_for_commit(); diff --git a/src/realm/impl/transact_log.cpp b/src/realm/impl/transact_log.cpp index 0e8af7c2ef8..6e42a2f3459 100644 --- a/src/realm/impl/transact_log.cpp +++ b/src/realm/impl/transact_log.cpp @@ -16,12 +16,9 @@ * **************************************************************************/ -#include #include -#include -namespace realm { -namespace _impl { +namespace realm::_impl { bool TransactLogEncoder::select_table(TableKey key) @@ -33,33 +30,7 @@ bool TransactLogEncoder::select_table(TableKey key) bool TransactLogEncoder::select_collection(ColKey col_key, ObjKey key) { - append_simple_instr(instr_SelectList, col_key, key.value); // Throws - return true; -} - -/******************************** Dictionary *********************************/ - -bool TransactLogEncoder::dictionary_insert(size_t dict_ndx, Mixed key) -{ - REALM_ASSERT(key.get_type() == type_String); - append_string_instr(instr_DictionaryInsert, key.get_string()); // Throws - append_simple_instr(dict_ndx); - return true; -} - -bool TransactLogEncoder::dictionary_set(size_t dict_ndx, Mixed key) -{ - REALM_ASSERT(key.get_type() == type_String); - append_string_instr(instr_DictionarySet, key.get_string()); // Throws - append_simple_instr(dict_ndx); - return true; -} - -bool TransactLogEncoder::dictionary_erase(size_t ndx, Mixed key) -{ - REALM_ASSERT(key.get_type() == type_String); - append_string_instr(instr_DictionaryErase, key.get_string()); // Throws - append_simple_instr(ndx); + append_simple_instr(instr_SelectCollection, col_key, key.value); // Throws return true; } @@ -69,5 +40,4 @@ void TransactLogParser::parser_error() const throw Exception(ErrorCodes::BadChangeset, "Bad transaction log"); } -} // namespace _impl -} // namespace realm +} // namespace realm::_impl diff --git a/src/realm/impl/transact_log.hpp b/src/realm/impl/transact_log.hpp index b93569298b9..093181335fd 100644 --- a/src/realm/impl/transact_log.hpp +++ b/src/realm/impl/transact_log.hpp @@ -27,9 +27,6 @@ #include #include -#include -#include - namespace realm { struct GlobalKey; @@ -55,23 +52,24 @@ enum Instruction { instr_RenameColumn = 22, // Rename column in selected descriptor // instr_SetLinkType = 23, Strong/weak (unused from file format 11) - instr_SelectList = 30, - instr_ListInsert = 31, // Insert list entry - instr_ListSet = 32, // Assign to list entry - instr_ListMove = 33, // Move an entry within a link list + instr_SelectCollection = 30, + instr_CollectionInsert = 31, // Insert collection entry + instr_CollectionSet = 32, // Assign to collection entry + instr_CollectionMove = 33, // Move an entry within an ordered collection // instr_ListSwap = 34, Swap two entries within a list (unused from file format 11) - instr_ListErase = 35, // Remove an entry from a list - instr_ListClear = 36, // Remove all entries from a list + instr_CollectionErase = 35, // Remove an entry from a collection + instr_CollectionClear = 36, // Remove all entries from a collection + // No longer emitted, but supported for a file shared with an older version. + // Treated identically to the Collection versions. instr_DictionaryInsert = 37, instr_DictionarySet = 38, instr_DictionaryErase = 39, + instr_SetInsert = 40, + instr_SetErase = 41, + instr_SetClear = 42, - instr_SetInsert = 40, // Insert value into set - instr_SetErase = 41, // Erase value from set - instr_SetClear = 42, // Remove all values in a set - - // An action involving TypedLinks has occured which caused + // An action involving TypedLinks has occurred which caused // the number of backlink columns to change. This can happen // when a TypedLink is created for the first time to a Table. instr_TypedLinkChange = 43, @@ -133,10 +131,6 @@ class NullInstructionObserver { { return true; } - bool select_link_list(ColKey, ObjKey) - { - return true; - } bool insert_group_level_table(TableKey) { return true; @@ -149,6 +143,10 @@ class NullInstructionObserver { { return true; } + bool typed_link_change(ColKey, TableKey) + { + return true; + } // Must have table selected: bool create_object(ObjKey) @@ -163,27 +161,6 @@ class NullInstructionObserver { { return true; } - bool list_set(size_t) - { - return true; - } - bool list_insert(size_t) - { - return true; - } - - bool dictionary_insert(size_t, Mixed) - { - return true; - } - bool dictionary_set(size_t, Mixed) - { - return true; - } - bool dictionary_erase(size_t, Mixed) - { - return true; - } // Must have descriptor selected: bool insert_column(ColKey) @@ -203,34 +180,24 @@ class NullInstructionObserver { return true; } - // Must have linklist selected: - bool list_move(size_t, size_t) - { - return true; - } - bool list_erase(size_t) + // Must have collection selected: + bool collection_set(size_t) { return true; } - bool list_clear(size_t) + bool collection_insert(size_t) { return true; } - - bool set_insert(size_t) + bool collection_move(size_t, size_t) { return true; } - bool set_erase(size_t) + bool collection_erase(size_t) { return true; } - bool set_clear(size_t) - { - return true; - } - - bool typed_link_change(ColKey, TableKey) + bool collection_clear(size_t) { return true; } @@ -275,20 +242,11 @@ class TransactLogEncoder { // Must have collection selected: bool select_collection(ColKey col_key, ObjKey key); - bool list_set(size_t list_ndx); - bool list_insert(size_t ndx); - bool list_move(size_t from_link_ndx, size_t to_link_ndx); - bool list_erase(size_t list_ndx); - bool list_clear(size_t old_list_size); - - // Must have set selected: - bool set_insert(size_t set_ndx); - bool set_erase(size_t set_ndx); - bool set_clear(size_t set_ndx); - - bool dictionary_insert(size_t dict_ndx, Mixed key); - bool dictionary_set(size_t dict_ndx, Mixed key); - bool dictionary_erase(size_t dict_ndx, Mixed key); + bool collection_set(size_t collection_ndx); + bool collection_insert(size_t ndx); + bool collection_move(size_t from_ndx, size_t to_ndx); + bool collection_erase(size_t collection_ndx); + bool collection_clear(size_t old_size); bool typed_link_change(ColKey col, TableKey dest); @@ -666,64 +624,40 @@ inline bool TransactLogEncoder::modify_object(ColKey col_key, ObjKey key) } +/************************************ Collections ***********************************/ -/************************************ List ***********************************/ - -inline bool TransactLogEncoder::list_set(size_t list_ndx) -{ - append_simple_instr(instr_ListSet, list_ndx); // Throws - return true; -} - -inline bool TransactLogEncoder::list_insert(size_t list_ndx) -{ - append_simple_instr(instr_ListInsert, list_ndx); // Throws - return true; -} - - -/************************************ Set ************************************/ - -inline bool TransactLogEncoder::set_insert(size_t set_ndx) +inline bool TransactLogEncoder::collection_set(size_t ndx) { - append_simple_instr(instr_SetInsert, set_ndx); // Throws + append_simple_instr(instr_CollectionSet, ndx); // Throws return true; } - -inline bool TransactLogEncoder::set_erase(size_t set_ndx) +inline bool TransactLogEncoder::collection_insert(size_t ndx) { - append_simple_instr(instr_SetErase, set_ndx); // Throws + append_simple_instr(instr_CollectionInsert, ndx); // Throws return true; } -inline bool TransactLogEncoder::set_clear(size_t set_size) -{ - append_simple_instr(instr_SetClear, set_size); // Throws - return true; -} - - -inline bool TransactLogEncoder::list_move(size_t from_link_ndx, size_t to_link_ndx) +inline bool TransactLogEncoder::collection_move(size_t from_ndx, size_t to_ndx) { // This test is to prevent some fuzzy testing on the server to crash - if (from_link_ndx != to_link_ndx) { - append_simple_instr(instr_ListMove, from_link_ndx, to_link_ndx); // Throws + if (from_ndx != to_ndx) { + append_simple_instr(instr_CollectionMove, from_ndx, to_ndx); // Throws } return true; } -inline bool TransactLogEncoder::list_erase(size_t list_ndx) +inline bool TransactLogEncoder::collection_erase(size_t ndx) { - append_simple_instr(instr_ListErase, list_ndx); // Throws + append_simple_instr(instr_CollectionErase, ndx); // Throws return true; } -inline bool TransactLogEncoder::list_clear(size_t old_list_size) +inline bool TransactLogEncoder::collection_clear(size_t old_size) { - append_simple_instr(instr_ListClear, old_list_size); // Throws + append_simple_instr(instr_CollectionClear, old_size); // Throws return true; } @@ -783,12 +717,6 @@ void TransactLogParser::parse_one(InstructionHandler& handler) case instr_SetDefault: // Should not appear in the transaction log parser_error(); - case instr_ListSet: { - size_t list_ndx = read_int(); - if (!handler.list_set(list_ndx)) // Throws - parser_error(); - return; - } case instr_CreateObject: { ObjKey key(read_int()); // Throws if (!handler.create_object(key)) // Throws @@ -809,77 +737,68 @@ void TransactLogParser::parse_one(InstructionHandler& handler) parser_error(); return; } - case instr_ListInsert: { - size_t list_ndx = read_int(); - if (!handler.list_insert(list_ndx)) // Throws + case instr_CollectionSet: { + size_t ndx = read_int(); + if (!handler.collection_set(ndx)) // Throws + parser_error(); + return; + } + case instr_SetInsert: + case instr_CollectionInsert: { + size_t ndx = read_int(); + if (!handler.collection_insert(ndx)) // Throws parser_error(); return; } - case instr_ListMove: { - size_t from_link_ndx = read_int(); // Throws - size_t to_link_ndx = read_int(); // Throws - if (!handler.list_move(from_link_ndx, to_link_ndx)) // Throws + case instr_CollectionMove: { + size_t from_ndx = read_int(); // Throws + size_t to_ndx = read_int(); // Throws + if (!handler.collection_move(from_ndx, to_ndx)) // Throws parser_error(); return; } - case instr_ListErase: { - size_t link_ndx = read_int(); // Throws - if (!handler.list_erase(link_ndx)) // Throws + case instr_SetErase: + case instr_CollectionErase: { + size_t ndx = read_int(); // Throws + if (!handler.collection_erase(ndx)) // Throws parser_error(); return; } - case instr_ListClear: { - size_t old_list_size = read_int(); // Throws - if (!handler.list_clear(old_list_size)) // Throws + case instr_SetClear: + case instr_CollectionClear: { + size_t old_size = read_int(); // Throws + if (!handler.collection_clear(old_size)) // Throws parser_error(); return; } case instr_DictionaryInsert: { int type = read_int(); // Throws REALM_ASSERT(type == int(type_String)); - Mixed key = Mixed(read_string(m_string_buffer)); - size_t dict_ndx = read_int(); // Throws - if (!handler.dictionary_insert(dict_ndx, key)) // Throws + read_string(m_string_buffer); // skip key + size_t dict_ndx = read_int(); // Throws + if (!handler.collection_insert(dict_ndx)) // Throws parser_error(); return; } case instr_DictionarySet: { int type = read_int(); // Throws REALM_ASSERT(type == int(type_String)); - Mixed key = Mixed(read_string(m_string_buffer)); + read_string(m_string_buffer); // skip key size_t dict_ndx = read_int(); // Throws - if (!handler.dictionary_set(dict_ndx, key)) // Throws + if (!handler.collection_set(dict_ndx)) // Throws parser_error(); return; } case instr_DictionaryErase: { int type = read_int(); // Throws REALM_ASSERT(type == int(type_String)); - Mixed key = Mixed(read_string(m_string_buffer)); + read_string(m_string_buffer); // skip key size_t dict_ndx = read_int(); // Throws - if (!handler.dictionary_erase(dict_ndx, key)) // Throws - parser_error(); - return; - } - case instr_SetInsert: { - size_t set_ndx = read_int(); // Throws - if (!handler.set_insert(set_ndx)) // Throws - parser_error(); - return; - } - case instr_SetErase: { - size_t set_ndx = read_int(); // Throws - if (!handler.set_erase(set_ndx)) // Throws + if (!handler.collection_erase(dict_ndx)) // Throws parser_error(); return; } - case instr_SetClear: { - size_t set_size = read_int(); // Throws - if (!handler.set_clear(set_size)) // Throws - parser_error(); - return; - } - case instr_SelectList: { + case instr_SelectCollection: { ColKey col_key = ColKey(read_int()); // Throws ObjKey key = ObjKey(read_int()); // Throws if (!handler.select_collection(col_key, key)) // Throws @@ -1033,269 +952,6 @@ inline bool TransactLogParser::read_char(char& c) return true; } - -class TransactReverser { -public: - bool select_table(TableKey key) - { - sync_table(); - m_encoder.select_table(key); - m_pending_ts_instr = get_inst(); - return true; - } - - bool insert_group_level_table(TableKey table_key) - { - sync_table(); - m_encoder.erase_class(table_key); - append_instruction(); - return true; - } - - bool erase_class(TableKey table_key) - { - sync_table(); - m_encoder.insert_group_level_table(table_key); - append_instruction(); - return true; - } - - bool rename_class(TableKey) - { - sync_table(); - return true; - } - - bool create_object(ObjKey key) - { - m_encoder.remove_object(key); // Throws - append_instruction(); - return true; - } - - bool remove_object(ObjKey key) - { - m_encoder.create_object(key); // Throws - append_instruction(); - return true; - } - - bool modify_object(ColKey col_key, ObjKey key) - { - m_encoder.modify_object(col_key, key); - append_instruction(); - return true; - } - - bool list_set(size_t ndx) - { - m_encoder.list_set(ndx); - append_instruction(); - return true; - } - - bool list_insert(size_t ndx) - { - m_encoder.list_erase(ndx); - append_instruction(); - return true; - } - - bool dictionary_insert(size_t dict_ndx, Mixed key) - { - m_encoder.dictionary_erase(dict_ndx, key); - return true; - } - - bool dictionary_set(size_t dict_ndx, Mixed key) - { - m_encoder.dictionary_set(dict_ndx, key); - return true; - } - - bool dictionary_erase(size_t dict_ndx, Mixed key) - { - m_encoder.dictionary_insert(dict_ndx, key); - return true; - } - - bool set_link_type(ColKey key) - { - m_encoder.set_link_type(key); - return true; - } - - bool insert_column(ColKey col_key) - { - m_encoder.erase_column(col_key); - append_instruction(); - return true; - } - - bool erase_column(ColKey col_key) - { - m_encoder.insert_column(col_key); - append_instruction(); - return true; - } - - bool rename_column(ColKey col_key) - { - m_encoder.rename_column(col_key); - return true; - } - - bool select_collection(ColKey col_key, ObjKey key) - { - sync_list(); - m_encoder.select_collection(col_key, key); - m_pending_ls_instr = get_inst(); - return true; - } - - bool list_move(size_t from_link_ndx, size_t to_link_ndx) - { - m_encoder.list_move(from_link_ndx, to_link_ndx); - append_instruction(); - return true; - } - - bool list_erase(size_t list_ndx) - { - m_encoder.list_insert(list_ndx); - append_instruction(); - return true; - } - - bool list_clear(size_t old_list_size) - { - // Append in reverse order because the reversed log is itself applied - // in reverse, and this way it generates all back-insertions rather than - // all front-insertions - for (size_t i = old_list_size; i > 0; --i) { - m_encoder.list_insert(i - 1); - append_instruction(); - } - return true; - } - - bool set_insert(size_t ndx) - { - m_encoder.set_erase(ndx); - append_instruction(); - return true; - } - - bool set_erase(size_t ndx) - { - m_encoder.set_insert(ndx); - append_instruction(); - return true; - } - - bool set_clear(size_t old_set_size) - { - for (size_t i = old_set_size; i > 0; --i) { - m_encoder.set_insert(i - 1); - append_instruction(); - } - return true; - } - - bool typed_link_change(ColKey col, TableKey dest) - { - m_encoder.typed_link_change(col, dest); - append_instruction(); - return true; - } - -private: - _impl::TransactLogBufferStream m_buffer; - _impl::TransactLogEncoder m_encoder{m_buffer}; - struct Instr { - size_t begin; - size_t end; - }; - std::vector m_instructions; - size_t current_instr_start = 0; - Instr m_pending_ts_instr{0, 0}; - Instr m_pending_ls_instr{0, 0}; - - Instr get_inst() - { - Instr instr; - instr.begin = current_instr_start; - current_instr_start = transact_log_size(); - instr.end = current_instr_start; - return instr; - } - - size_t transact_log_size() const - { - REALM_ASSERT_3(m_encoder.write_position(), >=, m_buffer.get_data()); - return m_encoder.write_position() - m_buffer.get_data(); - } - - void append_instruction() - { - m_instructions.push_back(get_inst()); - } - - void append_instruction(Instr instr) - { - m_instructions.push_back(instr); - } - - void sync_select(Instr& pending_instr) - { - if (pending_instr.begin != pending_instr.end) { - append_instruction(pending_instr); - pending_instr = {0, 0}; - } - } - - void sync_list() - { - sync_select(m_pending_ls_instr); - } - - void sync_table() - { - sync_list(); - sync_select(m_pending_ts_instr); - } - - friend class ReversedNoCopyInputStream; -}; - - -class ReversedNoCopyInputStream : public util::NoCopyInputStream { -public: - ReversedNoCopyInputStream(TransactReverser& reverser) - : m_instr_order(reverser.m_instructions) - { - // push any pending select_table into the buffer - reverser.sync_table(); - - m_buffer = reverser.m_buffer.get_data(); - m_current = m_instr_order.size(); - } - - util::Span next_block() override - { - if (m_current != 0) { - m_current--; - return {m_buffer + m_instr_order[m_current].begin, m_buffer + m_instr_order[m_current].end}; - } - return {m_buffer, m_buffer}; - } - -private: - const char* m_buffer; - std::vector& m_instr_order; - size_t m_current; -}; - } // namespace _impl } // namespace realm diff --git a/src/realm/object-store/audit.mm b/src/realm/object-store/audit.mm index 49b86222d90..28a6bd58208 100644 --- a/src/realm/object-store/audit.mm +++ b/src/realm/object-store/audit.mm @@ -292,17 +292,11 @@ bool select_collection(ColKey, ObjKey obj) noexcept // clang-format off // We don't care about fine-grained changes to collections and just do // object-level change tracking, which is covered by select_collection() - bool list_set(size_t) { return true; } - bool list_insert(size_t) { return true; } - bool list_move(size_t, size_t) { return true; } - bool list_erase(size_t) { return true; } - bool list_clear(size_t) { return true; } - bool dictionary_insert(size_t, Mixed const&) { return true; } - bool dictionary_set(size_t, Mixed const&) { return true; } - bool dictionary_erase(size_t, Mixed const&) { return true; } - bool set_insert(size_t) { return true; } - bool set_erase(size_t) { return true; } - bool set_clear(size_t) { return true; } + bool collection_set(size_t) { return true; } + bool collection_insert(size_t) { return true; } + bool collection_move(size_t, size_t) { return true; } + bool collection_erase(size_t) { return true; } + bool collection_clear(size_t) { return true; } // We don't run this code on arbitrary transactions that could perform schema changes bool insert_group_level_table(TableKey) { unexpected_instruction(); } diff --git a/src/realm/object-store/impl/collection_notifier.hpp b/src/realm/object-store/impl/collection_notifier.hpp index 97ac005e80e..49f8ff638d0 100644 --- a/src/realm/object-store/impl/collection_notifier.hpp +++ b/src/realm/object-store/impl/collection_notifier.hpp @@ -171,6 +171,11 @@ class CollectionNotifier { return m_have_callbacks; } + Transaction& transaction() const noexcept + { + return *m_transaction; + } + protected: void add_changes(CollectionChangeBuilder change) REQUIRES(!m_callback_mutex); std::unique_lock lock_target(); @@ -200,11 +205,6 @@ class CollectionNotifier { void update_related_tables(Table const& table) REQUIRES(m_callback_mutex); - Transaction& transaction() const noexcept - { - return *m_transaction; - } - // The actual change, calculated in run() and delivered in prepare_handover() CollectionChangeBuilder m_change; diff --git a/src/realm/object-store/impl/deep_change_checker.hpp b/src/realm/object-store/impl/deep_change_checker.hpp index 0f1a5d4df3a..5ed89deafa6 100644 --- a/src/realm/object-store/impl/deep_change_checker.hpp +++ b/src/realm/object-store/impl/deep_change_checker.hpp @@ -40,7 +40,7 @@ using ref_type = size_t; namespace _impl { class RealmCoordinator; -struct ListChangeInfo { +struct CollectionChangeInfo { TableKey table_key; ObjKey obj_key; ColKey col_key; @@ -48,9 +48,8 @@ struct ListChangeInfo { }; struct TransactionChangeInfo { - std::vector lists; + std::vector collections; std::unordered_map tables; - bool track_all; bool schema_changed; }; diff --git a/src/realm/object-store/impl/list_notifier.cpp b/src/realm/object-store/impl/list_notifier.cpp index 66913d3eaa2..42707113835 100644 --- a/src/realm/object-store/impl/list_notifier.cpp +++ b/src/realm/object-store/impl/list_notifier.cpp @@ -61,7 +61,8 @@ bool ListNotifier::do_add_required_change_info(TransactionChangeInfo& info) if (!m_list || !m_list->is_attached()) return false; // origin row was deleted after the notification was added - info.lists.push_back({m_list->get_table()->get_key(), m_list->get_owner_key(), m_list->get_col_key(), &m_change}); + info.collections.push_back( + {m_list->get_table()->get_key(), m_list->get_owner_key(), m_list->get_col_key(), &m_change}); m_info = &info; diff --git a/src/realm/object-store/impl/realm_coordinator.cpp b/src/realm/object-store/impl/realm_coordinator.cpp index c3a8a12760f..9d1d7951707 100644 --- a/src/realm/object-store/impl/realm_coordinator.cpp +++ b/src/realm/object-store/impl/realm_coordinator.cpp @@ -844,106 +844,6 @@ void RealmCoordinator::on_change() } } -namespace { -bool compare_notifier_versions(const std::shared_ptr<_impl::CollectionNotifier>& a, - const std::shared_ptr<_impl::CollectionNotifier>& b) -{ - return a->version() < b->version(); -} - -class IncrementalChangeInfo { -public: - IncrementalChangeInfo(Transaction& tr, std::vector>& notifiers) - : m_tr(tr) - { - if (notifiers.empty()) - return; - - // Sort the notifiers by their source version so that we can pull them - // all forward to the latest version in a single pass over the transaction log - std::sort(notifiers.begin(), notifiers.end(), compare_notifier_versions); - - // Preallocate the required amount of space in the vector so that we can - // safely give out pointers to within the vector - size_t count = 1; - for (auto it = notifiers.begin(), next = it + 1; next != notifiers.end(); ++it, ++next) { - if (compare_notifier_versions(*it, *next)) - ++count; - } - m_info.reserve(count); - m_info.resize(1); - m_current = &m_info[0]; - } - - TransactionChangeInfo& current() const - { - return *m_current; - } - - bool advance_incremental(VersionID version) - { - if (version != m_tr.get_version_of_current_transaction()) { - transaction::advance(m_tr, *m_current, version); - m_info.push_back({std::move(m_current->lists)}); - auto next = &m_info.back(); - for (auto& table : m_current->tables) - next->tables[table.first]; - m_current = next; - return true; - } - return false; - } - - void advance_to_final(VersionID version) - { - if (!m_current) { - transaction::advance(m_tr, nullptr, version); - return; - } - - transaction::advance(m_tr, *m_current, version); - - // We now need to combine the transaction change info objects so that all of - // the notifiers see the complete set of changes from their first version to - // the most recent one - for (size_t i = m_info.size() - 1; i > 0; --i) { - auto& cur = m_info[i]; - if (cur.tables.empty()) - continue; - auto& prev = m_info[i - 1]; - if (prev.tables.empty()) { - prev.tables = cur.tables; - continue; - } - for (auto& ct : cur.tables) { - auto& pt = prev.tables[ct.first]; - if (pt.empty()) - pt = ct.second; - else - pt.merge(ObjectChangeSet{ct.second}); - } - } - - // Copy the list change info if there are multiple LinkViews for the same LinkList - auto id = [](auto const& list) { - return std::tie(list.table_key, list.col_key, list.obj_key); - }; - for (size_t i = 1; i < m_current->lists.size(); ++i) { - for (size_t j = i; j > 0; --j) { - if (id(m_current->lists[i]) == id(m_current->lists[j - 1])) { - m_current->lists[j - 1].changes->merge(CollectionChangeBuilder{*m_current->lists[i].changes}); - } - } - } - } - -private: - std::vector m_info; - TransactionChangeInfo* m_current = nullptr; - Transaction& m_tr; -}; -} // anonymous namespace - void RealmCoordinator::run_async_notifiers() { util::CheckedUniqueLock lock(m_notifier_mutex); @@ -992,47 +892,26 @@ void RealmCoordinator::run_async_notifiers() } else { REALM_ASSERT(!skip_version); + if (m_new_notifiers.empty()) { + // We were spuriously woken up and there isn't actually anything to do + return; + } } auto new_notifiers = std::move(m_new_notifiers); m_new_notifiers.clear(); m_notifiers.insert(m_notifiers.end(), new_notifiers.begin(), new_notifiers.end()); + lock.unlock(); // Advance all of the new notifiers to the most recent version, if any - TransactionRef new_notifier_transaction; - util::Optional new_notifier_change_info; + std::vector new_notifier_change_info; if (!new_notifiers.empty()) { - lock.unlock(); - - // Starting from the oldest notifier, incrementally advance the notifiers - // to the latest version, attaching each new notifier as we reach its - // source version. Suppose three new notifiers have been created: - // - Notifier A has a source version of 2 - // - Notifier B has a source version of 7 - // - Notifier C has a source version of 5 - // Notifier A wants the changes from versions 2-latest, B wants 7-latest, - // and C wants 5-latest. We achieve this by starting at version 2 and - // attaching A, then advancing to version 5 (letting A gather changes - // from 2-5). We then attach C and advance to 7, then attach B and advance - // to the latest. - std::sort(new_notifiers.begin(), new_notifiers.end(), compare_notifier_versions); - new_notifier_transaction = m_db->start_read(new_notifiers.front()->version()); - new_notifier_change_info.emplace(*new_notifier_transaction, new_notifiers); + new_notifier_change_info.reserve(new_notifiers.size()); for (auto& notifier : new_notifiers) { - new_notifier_change_info->advance_incremental(notifier->version()); - notifier->attach_to(new_notifier_transaction); - notifier->add_required_change_info(new_notifier_change_info->current()); + new_notifier_change_info.emplace_back(); + notifier->add_required_change_info(new_notifier_change_info.back()); + transaction::advance(notifier->transaction(), new_notifier_change_info.back(), version); } - new_notifier_change_info->advance_to_final(version); - } - else { - if (version == m_notifier_transaction->get_version_of_current_transaction()) { - // We were spuriously woken up and there isn't actually anything to do - REALM_ASSERT(!skip_version); - return; - } - - lock.unlock(); } // If the skip version is set and we have more than one version to process, @@ -1045,10 +924,10 @@ void RealmCoordinator::run_async_notifiers() if (skip_version && skip_version->get_version_of_current_transaction() != version) { REALM_ASSERT(!notifiers.empty()); REALM_ASSERT(version >= skip_version->get_version_of_current_transaction()); - IncrementalChangeInfo change_info(*m_notifier_transaction, notifiers); + TransactionChangeInfo info; for (auto& notifier : notifiers) - notifier->add_required_change_info(change_info.current()); - change_info.advance_to_final(skip_version->get_version_of_current_transaction()); + notifier->add_required_change_info(info); + transaction::advance(*m_notifier_transaction, info, skip_version->get_version_of_current_transaction()); for (auto& notifier : notifiers) notifier->run(); @@ -1059,17 +938,37 @@ void RealmCoordinator::run_async_notifiers() // Advance the non-new notifiers to the same version as we advanced the new // ones to (or the latest if there were no new ones) - IncrementalChangeInfo change_info(*m_notifier_transaction, notifiers); + TransactionChangeInfo change_info; for (auto& notifier : notifiers) { - notifier->add_required_change_info(change_info.current()); + notifier->add_required_change_info(change_info); + } + transaction::advance(*m_notifier_transaction, change_info, version); + + { + // If there's multiple notifiers for a single collection, we only populate + // the data for the first one during parsing and need to copy it to the + // others. This is a reverse scan where each collection looks for the + // first collection with the same id. It is O(N^2), but typically the + // number of collections observed will be very small. + auto id = [](auto const& c) { + return std::tie(c.table_key, c.col_key, c.obj_key); + }; + auto& collections = change_info.collections; + for (size_t i = collections.size(); i > 0; --i) { + for (size_t j = 0; j < i - 1; ++j) { + if (id(collections[i - 1]) == id(collections[j])) { + collections[i - 1].changes->merge(CollectionChangeBuilder{*collections[j].changes}); + break; + } + } + } } - change_info.advance_to_final(version); // Now that they're at the same version, switch the new notifiers over to // the main Transaction used for background work rather than the temporary one - REALM_ASSERT(new_notifiers.empty() || m_notifier_transaction->get_version_of_current_transaction() == - new_notifier_transaction->get_version_of_current_transaction()); for (auto& notifier : new_notifiers) { + REALM_ASSERT(m_notifier_transaction->get_version_of_current_transaction() == + notifier->transaction().get_version_of_current_transaction()); notifier->attach_to(m_notifier_transaction); notifier->run(); } diff --git a/src/realm/object-store/impl/results_notifier.cpp b/src/realm/object-store/impl/results_notifier.cpp index 66b4ec86af8..4a0d565fdce 100644 --- a/src/realm/object-store/impl/results_notifier.cpp +++ b/src/realm/object-store/impl/results_notifier.cpp @@ -279,7 +279,8 @@ bool ListResultsNotifier::do_add_required_change_info(TransactionChangeInfo& inf if (!m_list->is_attached()) return false; // origin row was deleted after the notification was added - info.lists.push_back({m_list->get_table()->get_key(), m_list->get_owner_key(), m_list->get_col_key(), &m_change}); + info.collections.push_back( + {m_list->get_table()->get_key(), m_list->get_owner_key(), m_list->get_col_key(), &m_change}); m_info = &info; return true; @@ -322,7 +323,7 @@ void ListResultsNotifier::calculate_changes() void ListResultsNotifier::run() { - if (!m_list->is_attached()) { + if (!m_list || !m_list->is_attached()) { // List was deleted, so report all of the rows being removed m_change = {}; m_change.deletions.set(m_previous_indices.size()); diff --git a/src/realm/object-store/impl/transact_log_handler.cpp b/src/realm/object-store/impl/transact_log_handler.cpp index 80f2f22ad77..36f617febbc 100644 --- a/src/realm/object-store/impl/transact_log_handler.cpp +++ b/src/realm/object-store/impl/transact_log_handler.cpp @@ -34,7 +34,7 @@ namespace { class KVOAdapter : public _impl::TransactionChangeInfo { public: - KVOAdapter(std::vector& observers, BindingContext* context); + KVOAdapter(std::vector& observers, BindingContext* context, bool is_rollback); void before(Transaction& sg); void after(Transaction& sg); @@ -51,12 +51,15 @@ class KVOAdapter : public _impl::TransactionChangeInfo { }; std::vector m_lists; VersionID m_version; + bool m_rollback; }; -KVOAdapter::KVOAdapter(std::vector& observers, BindingContext* context) +KVOAdapter::KVOAdapter(std::vector& observers, BindingContext* context, + bool is_rollback) : _impl::TransactionChangeInfo{} , m_context(context) , m_observers(observers) + , m_rollback(is_rollback) { if (m_observers.empty()) return; @@ -82,7 +85,7 @@ KVOAdapter::KVOAdapter(std::vector& observers, Bi for (auto& tbl : tables_needed) tables[tbl] = {}; for (auto& list : m_lists) - lists.push_back({list.observer->table_key, list.observer->obj_key, list.col, &list.builder}); + collections.push_back({list.observer->table_key, list.observer->obj_key, list.col, &list.builder}); } void KVOAdapter::before(Transaction& sg) @@ -101,7 +104,7 @@ void KVOAdapter::before(Transaction& sg) auto const& table = it->second; auto key = observer.obj_key; - if (table.deletions_contains(key)) { + if (m_rollback ? table.insertions_contains(key) : table.deletions_contains(key)) { m_invalidated.push_back(observer.info); continue; } @@ -134,7 +137,7 @@ void KVOAdapter::before(Transaction& sg) builder.modifications.remove(builder.insertions); - // KVO can't express moves (becaue NSArray doesn't have them), so + // KVO can't express moves (because NSArray doesn't have them), so // transform them into a series of sets on each affected index when possible if (!builder.moves.empty() && builder.insertions.count() == builder.moves.size() && builder.deletions.count() == builder.moves.size()) { @@ -185,6 +188,22 @@ void KVOAdapter::before(Transaction& sg) changes.kind = BindingContext::ColumnInfo::Kind::Remove; changes.indices = builder.deletions; } + + // If we're rolling back a write transaction, insertions are actually + // deletions and vice-versa. More complicated scenarios which would + // require logic beyond this fortunately just aren't supported by KVO. + if (m_rollback) { + switch (changes.kind) { + case BindingContext::ColumnInfo::Kind::Insert: + changes.kind = BindingContext::ColumnInfo::Kind::Remove; + break; + case BindingContext::ColumnInfo::Kind::Remove: + changes.kind = BindingContext::ColumnInfo::Kind::Insert; + break; + default: + break; + } + } } m_context->will_change(m_observers, m_invalidated); } @@ -263,51 +282,27 @@ class TransactLogValidationMixin { { return true; } - bool list_set(size_t) - { - return true; - } - bool list_insert(size_t) - { - return true; - } - bool list_erase(size_t) + bool collection_set(size_t) { return true; } - bool list_clear(size_t) + bool collection_insert(size_t) { return true; } - bool list_move(size_t, size_t) + bool collection_erase(size_t) { return true; } - bool list_swap(size_t, size_t) + bool collection_clear(size_t) { return true; } - bool dictionary_insert(size_t, Mixed) + bool collection_move(size_t, size_t) { return true; } - bool dictionary_set(size_t, Mixed) - { - return true; - } - bool dictionary_erase(size_t, Mixed) - { - return true; - } - bool set_insert(size_t) - { - return true; - } - bool set_erase(size_t) - { - return true; - } - bool set_clear(size_t) + bool collection_swap(size_t, size_t) { return true; } @@ -337,19 +332,6 @@ class TransactLogObserver : public TransactLogValidationMixin { _impl::CollectionChangeBuilder* m_active_collection = nullptr; ObjectChangeSet* m_active_table = nullptr; - _impl::CollectionChangeBuilder* find_list(ObjKey obj, ColKey col) - { - // When there are multiple source versions there could be multiple - // change objects for a single LinkView, in which case we need to use - // the last one - auto table = current_table(); - for (auto it = m_info.lists.rbegin(), end = m_info.lists.rend(); it != end; ++it) { - if (it->table_key == table && it->obj_key == obj && it->col_key == col) - return it->changes; - } - return nullptr; - } - public: TransactLogObserver(_impl::TransactionChangeInfo& info) : m_info(info) @@ -358,8 +340,8 @@ class TransactLogObserver : public TransactLogValidationMixin { void parse_complete() { - for (auto& list : m_info.lists) - list.changes->clean_up_stale_moves(); + for (auto& collection : m_info.collections) + collection.changes->clean_up_stale_moves(); for (auto it = m_info.tables.begin(); it != m_info.tables.end();) { if (it->second.empty()) it = m_info.tables.erase(it); @@ -373,47 +355,49 @@ class TransactLogObserver : public TransactLogValidationMixin { TransactLogValidationMixin::select_table(key); TableKey table_key = current_table(); - if (m_info.track_all) - m_active_table = &m_info.tables[table_key]; - else { - auto it = m_info.tables.find(table_key); - if (it == m_info.tables.end()) - m_active_table = nullptr; - else - m_active_table = &it->second; - } + if (auto it = m_info.tables.find(table_key); it != m_info.tables.end()) + m_active_table = &it->second; + else + m_active_table = nullptr; return true; } bool select_collection(ColKey col, ObjKey obj) { modify_object(col, obj); - m_active_collection = find_list(obj, col); + auto table = current_table(); + for (auto& c : m_info.collections) { + if (c.table_key == table && c.obj_key == obj && c.col_key == col) { + m_active_collection = c.changes; + return true; + } + } + m_active_collection = nullptr; return true; } - bool list_set(size_t index) + bool collection_set(size_t index) { if (m_active_collection) m_active_collection->modify(index); return true; } - bool list_insert(size_t index) + bool collection_insert(size_t index) { if (m_active_collection) m_active_collection->insert(index); return true; } - bool list_erase(size_t index) + bool collection_erase(size_t index) { if (m_active_collection) m_active_collection->erase(index); return true; } - bool list_swap(size_t index1, size_t index2) + bool collection_swap(size_t index1, size_t index2) { if (m_active_collection) { if (index1 > index2) @@ -425,59 +409,20 @@ class TransactLogObserver : public TransactLogValidationMixin { return true; } - bool list_clear(size_t old_size) + bool collection_clear(size_t old_size) { if (m_active_collection) m_active_collection->clear(old_size); return true; } - bool list_move(size_t from, size_t to) + bool collection_move(size_t from, size_t to) { if (m_active_collection) m_active_collection->move(from, to); return true; } - bool set_insert(size_t index) - { - if (m_active_collection) - m_active_collection->insert(index); - return true; - } - bool set_erase(size_t index) - { - if (m_active_collection) - m_active_collection->erase(index); - return true; - } - bool set_clear(size_t old_size) - { - if (m_active_collection) - m_active_collection->clear(old_size); - return true; - } - bool dictionary_insert(size_t index, Mixed) - { - if (m_active_collection) { - m_active_collection->insert(index); - } - return true; - } - bool dictionary_set(size_t index, Mixed) - { - if (m_active_collection) { - m_active_collection->modify(index); - } - return true; - } - bool dictionary_erase(size_t index, Mixed) - { - if (m_active_collection) - m_active_collection->erase(index); - return true; - } - bool create_object(ObjKey key) { if (m_active_table) @@ -493,14 +438,14 @@ class TransactLogObserver : public TransactLogValidationMixin { m_active_table->deletions_add(key); m_active_table->modifications_remove(key); - for (size_t i = 0; i < m_info.lists.size(); ++i) { - auto& list = m_info.lists[i]; + for (size_t i = 0; i < m_info.collections.size(); ++i) { + auto& list = m_info.collections[i]; if (list.table_key != current_table()) continue; if (list.obj_key == key) { - if (i + 1 < m_info.lists.size()) - m_info.lists[i] = std::move(m_info.lists.back()); - m_info.lists.pop_back(); + if (i + 1 < m_info.collections.size()) + m_info.collections[i] = std::move(m_info.collections.back()); + m_info.collections.pop_back(); continue; } } @@ -541,9 +486,9 @@ class KVOTransactLogObserver : public TransactLogObserver { public: KVOTransactLogObserver(std::vector& observers, BindingContext* context, - _impl::NotifierPackage& notifiers, Transaction& sg) + _impl::NotifierPackage& notifiers, Transaction& sg, bool is_rollback = false) : TransactLogObserver(m_adapter) - , m_adapter(observers, context) + , m_adapter(observers, context, is_rollback) , m_notifiers(notifiers) , m_sg(sg) { @@ -663,13 +608,13 @@ void cancel(Transaction& tr, BindingContext* context) } _impl::NotifierPackage notifiers; - KVOTransactLogObserver o(observers, context, notifiers, tr); - tr.rollback_and_continue_as_read(&o); + KVOTransactLogObserver o(observers, context, notifiers, tr, true); + tr.rollback_and_continue_as_read(o); } void advance(Transaction& tr, TransactionChangeInfo& info, VersionID version) { - if (!info.track_all && info.tables.empty() && info.lists.empty()) { + if (info.tables.empty() && info.collections.empty()) { tr.advance_read(version); } else { diff --git a/src/realm/replication.cpp b/src/realm/replication.cpp index 8cc151f244d..074a78358bb 100644 --- a/src/realm/replication.cpp +++ b/src/realm/replication.cpp @@ -106,29 +106,29 @@ void Replication::do_select_collection(const CollectionBase& list) void Replication::list_clear(const CollectionBase& list) { select_collection(list); // Throws - m_encoder.list_clear(list.size()); // Throws + m_encoder.collection_clear(list.size()); // Throws } void Replication::link_list_nullify(const Lst& list, size_t link_ndx) { select_collection(list); - m_encoder.list_erase(link_ndx); + m_encoder.collection_erase(link_ndx); } -void Replication::dictionary_insert(const CollectionBase& dict, size_t ndx, Mixed key, Mixed) +void Replication::dictionary_insert(const CollectionBase& dict, size_t ndx, Mixed, Mixed) { select_collection(dict); - m_encoder.dictionary_insert(ndx, key); + m_encoder.collection_insert(ndx); } -void Replication::dictionary_set(const CollectionBase& dict, size_t ndx, Mixed key, Mixed) +void Replication::dictionary_set(const CollectionBase& dict, size_t ndx, Mixed, Mixed) { select_collection(dict); - m_encoder.dictionary_set(ndx, key); + m_encoder.collection_set(ndx); } -void Replication::dictionary_erase(const CollectionBase& dict, size_t ndx, Mixed key) +void Replication::dictionary_erase(const CollectionBase& dict, size_t ndx, Mixed) { select_collection(dict); - m_encoder.dictionary_erase(ndx, key); + m_encoder.collection_erase(ndx); } diff --git a/src/realm/replication.hpp b/src/realm/replication.hpp index b29e7b2127c..afe579cfb0f 100644 --- a/src/realm/replication.hpp +++ b/src/realm/replication.hpp @@ -49,7 +49,6 @@ class Replication { virtual void add_class(TableKey table_key, StringData table_name, Table::Type table_type); virtual void add_class_with_primary_key(TableKey, StringData table_name, DataType pk_type, StringData pk_field, bool nullable, Table::Type table_type); - virtual void prepare_erase_class(TableKey table_key); virtual void erase_class(TableKey table_key, size_t num_tables); virtual void rename_class(TableKey table_key, StringData new_name); virtual void insert_column(const Table*, ColKey col_key, DataType type, StringData name, Table* target_table); @@ -494,8 +493,6 @@ inline void Replication::select_collection(const CollectionBase& list) } } -inline void Replication::prepare_erase_class(TableKey) {} - inline void Replication::erase_class(TableKey table_key, size_t) { unselect_all(); @@ -554,31 +551,31 @@ inline void Replication::nullify_link(const Table* t, ColKey col_key, ObjKey key inline void Replication::list_set(const CollectionBase& list, size_t list_ndx, Mixed) { select_collection(list); // Throws - m_encoder.list_set(list.translate_index(list_ndx)); // Throws + m_encoder.collection_set(list.translate_index(list_ndx)); // Throws } inline void Replication::list_insert(const CollectionBase& list, size_t list_ndx, Mixed, size_t) { select_collection(list); // Throws - m_encoder.list_insert(list.translate_index(list_ndx)); // Throws + m_encoder.collection_insert(list.translate_index(list_ndx)); // Throws } inline void Replication::set_insert(const CollectionBase& set, size_t set_ndx, Mixed) { select_collection(set); // Throws - m_encoder.set_insert(set_ndx); // Throws + m_encoder.collection_insert(set_ndx); // Throws } inline void Replication::set_erase(const CollectionBase& set, size_t set_ndx, Mixed) { select_collection(set); // Throws - m_encoder.set_erase(set_ndx); // Throws + m_encoder.collection_erase(set_ndx); // Throws } inline void Replication::set_clear(const CollectionBase& set) { select_collection(set); // Throws - m_encoder.set_clear(set.size()); // Throws + m_encoder.collection_clear(set.size()); // Throws } inline void Replication::remove_object(const Table* t, ObjKey key) @@ -590,13 +587,13 @@ inline void Replication::remove_object(const Table* t, ObjKey key) inline void Replication::list_move(const CollectionBase& list, size_t from_link_ndx, size_t to_link_ndx) { select_collection(list); // Throws - m_encoder.list_move(list.translate_index(from_link_ndx), list.translate_index(to_link_ndx)); // Throws + m_encoder.collection_move(list.translate_index(from_link_ndx), list.translate_index(to_link_ndx)); // Throws } inline void Replication::list_erase(const CollectionBase& list, size_t link_ndx) { select_collection(list); // Throws - m_encoder.list_erase(list.translate_index(link_ndx)); // Throws + m_encoder.collection_erase(list.translate_index(link_ndx)); // Throws } inline void Replication::typed_link_change(const Table* source_table, ColKey col, TableKey dest_table) diff --git a/src/realm/sync/instruction_replication.cpp b/src/realm/sync/instruction_replication.cpp index ab9c0ad17da..25182975689 100644 --- a/src/realm/sync/instruction_replication.cpp +++ b/src/realm/sync/instruction_replication.cpp @@ -298,12 +298,6 @@ void SyncReplication::create_object_with_primary_key(const Table* table, ObjKey } -void SyncReplication::prepare_erase_class(TableKey table_key) -{ - REALM_ASSERT(!m_table_being_erased); - m_table_being_erased = table_key; -} - void SyncReplication::erase_class(TableKey table_key, size_t num_tables) { Replication::erase_class(table_key, num_tables); @@ -312,15 +306,10 @@ void SyncReplication::erase_class(TableKey table_key, size_t num_tables) bool is_class = m_transaction->table_is_public(table_key); - if (is_class) { - REALM_ASSERT(table_key == m_table_being_erased); - m_table_being_erased = TableKey(); - - if (!m_short_circuit) { - Instruction::EraseTable instr; - instr.table = emit_class_name(table_name); - emit(instr); - } + if (is_class && !m_short_circuit) { + Instruction::EraseTable instr; + instr.table = emit_class_name(table_name); + emit(instr); } m_last_table = nullptr; @@ -385,11 +374,6 @@ void SyncReplication::erase_column(const Table* table, ColKey col_ndx) Replication::erase_column(table, col_ndx); if (select_table(*table)) { - if (table->get_key() == m_table_being_erased) { - // Ignore any EraseColumn instructions generated by Core as part of - // EraseTable. - return; - } // Not allowed to remove PK/OID columns! REALM_ASSERT(col_ndx != table->get_primary_key_column()); Instruction::EraseColumn instr; diff --git a/src/realm/sync/instruction_replication.hpp b/src/realm/sync/instruction_replication.hpp index 5f37c919a07..44963679482 100644 --- a/src/realm/sync/instruction_replication.hpp +++ b/src/realm/sync/instruction_replication.hpp @@ -51,7 +51,6 @@ class SyncReplication : public Replication { void create_object(const Table*, GlobalKey) final; void create_object_with_primary_key(const Table*, ObjKey, Mixed) final; - void prepare_erase_class(TableKey tk) final; void erase_class(TableKey table_key, size_t num_tables) final; void rename_class(TableKey table_key, StringData new_name) final; void insert_column(const Table*, ColKey col_key, DataType type, StringData name, Table* target_table) final; @@ -105,8 +104,6 @@ class SyncReplication : public Replication { ChangesetEncoder m_encoder; Transaction* m_transaction; - TableKey m_table_being_erased; - template void emit(T instruction); diff --git a/src/realm/transaction.hpp b/src/realm/transaction.hpp index 7af3064aa73..4d3f28f5ce8 100644 --- a/src/realm/transaction.hpp +++ b/src/realm/transaction.hpp @@ -62,12 +62,8 @@ class Transaction : public Group { VersionID commit_and_continue_as_read(bool commit_to_disk = true) REQUIRES(!m_async_mutex); VersionID commit_and_continue_writing(); template - void rollback_and_continue_as_read(O* observer) REQUIRES(!m_async_mutex); - void rollback_and_continue_as_read() REQUIRES(!m_async_mutex) - { - _impl::NullInstructionObserver* o = nullptr; - rollback_and_continue_as_read(o); - } + void rollback_and_continue_as_read(O& observer) REQUIRES(!m_async_mutex); + void rollback_and_continue_as_read() REQUIRES(!m_async_mutex); template void advance_read(O* observer, VersionID target_version = VersionID()); void advance_read(VersionID target_version = VersionID()) @@ -415,30 +411,33 @@ inline bool Transaction::promote_to_write(O* observer, bool nonblocking) } template -inline void Transaction::rollback_and_continue_as_read(O* observer) +inline void Transaction::rollback_and_continue_as_read(O& observer) { if (m_transact_stage != DB::transact_Writing) throw WrongTransactionState("Not a write transaction"); - Replication* repl = db->get_replication(); if (!repl) throw IllegalOperation("No transaction log when rolling back"); BinaryData uncommitted_changes = repl->get_uncommitted_changes(); + if (uncommitted_changes.size()) { + util::SimpleInputStream in(uncommitted_changes); + _impl::TransactLogParser parser; // Throws + parser.parse(in, observer); // Throws + observer.parse_complete(); // Throws + } - // Possible optimization: We are currently creating two transaction log parsers, one here, - // and one in advance_transact(). That is wasteful as the parser creation is - // expensive. - util::SimpleInputStream in(uncommitted_changes); - _impl::TransactLogParser parser; // Throws - _impl::TransactReverser reverser; - parser.parse(in, reverser); // Throws + rollback_and_continue_as_read(); +} - if (observer && uncommitted_changes.size()) { - _impl::ReversedNoCopyInputStream reversed_in(reverser); - parser.parse(reversed_in, *observer); // Throws - observer->parse_complete(); // Throws - } +inline void Transaction::rollback_and_continue_as_read() +{ + if (m_transact_stage != DB::transact_Writing) + throw WrongTransactionState("Not a write transaction"); + + Replication* repl = db->get_replication(); + if (!repl) + throw IllegalOperation("No transaction log when rolling back"); // Mark all managed space (beyond the attached file) as free. db->reset_free_space_tracking(); // Throws @@ -447,11 +446,10 @@ inline void Transaction::rollback_and_continue_as_read(O* observer) ref_type top_ref = m_read_lock.m_top_ref; size_t file_size = m_read_lock.m_file_size; - _impl::ReversedNoCopyInputStream reversed_in(reverser); // since we had the write lock, we already have the latest encrypted pages in memory m_alloc.update_reader_view(file_size); // Throws update_allocator_wrappers(false); - advance_transact(top_ref, reversed_in, false); // Throws + advance_transact(top_ref, nullptr, false); // Throws if (!holds_write_mutex()) db->end_write_on_correct_thread(); @@ -510,7 +508,7 @@ inline bool Transaction::internal_advance_read(O* observer, VersionID version_id // part of the Realm state, then it would not have been necessary to retain // the old read lock beyond this point. _impl::ChangesetInputStream in(hist, old_version, new_version); - advance_transact(new_top_ref, in, writable); // Throws + advance_transact(new_top_ref, &in, writable); // Throws g.release(); db->release_read_lock(m_read_lock); m_read_lock = new_read_lock; diff --git a/test/object-store/transaction_log_parsing.cpp b/test/object-store/transaction_log_parsing.cpp index 686feed50af..2303ae2b993 100644 --- a/test/object-store/transaction_log_parsing.cpp +++ b/test/object-store/transaction_log_parsing.cpp @@ -59,10 +59,10 @@ class CaptureHelper { _impl::CollectionChangeBuilder c; _impl::TransactionChangeInfo info{}; info.tables[m_table_key]; - info.lists.push_back({m_table_key, m_list.get_owner_key(), m_list.get_col_key(), &c}); + info.collections.push_back({m_table_key, m_list.get_owner_key(), m_list.get_col_key(), &c}); _impl::transaction::advance(*m_group, info); - if (info.lists.empty()) { + if (info.collections.empty()) { REQUIRE(!m_list.is_attached()); return {}; } diff --git a/test/test_lang_bind_helper.cpp b/test/test_lang_bind_helper.cpp index cc98b6f353e..f23ce7e0fec 100644 --- a/test/test_lang_bind_helper.cpp +++ b/test/test_lang_bind_helper.cpp @@ -2017,13 +2017,6 @@ class NoOpTransactionLogParser { return true; } - bool select_link_list(ColKey col_key, ObjKey obj_key, size_t) - { - m_current_linkview_col = col_key; - m_current_linkview_row = obj_key; - return true; - } - bool select_collection(ColKey col_key, ObjKey obj_key) { m_current_linkview_col = col_key; @@ -2031,13 +2024,7 @@ class NoOpTransactionLogParser { return true; } - // subtables not supported - bool select_descriptor(int, const size_t*) - { - return false; - } - - // Default no-op implmentations of all of the mutation instructions + // Default no-op implementations of all of the mutation instructions bool insert_group_level_table(TableKey) { return false; @@ -2054,38 +2041,14 @@ class NoOpTransactionLogParser { { return false; } - bool insert_link_column(ColKey, DataType, StringData, size_t, size_t) - { - return false; - } bool erase_column(ColKey) { return false; } - bool erase_link_column(size_t, size_t, size_t) - { - return false; - } bool rename_column(ColKey) { return false; } - bool add_search_index(size_t) - { - return false; - } - bool remove_search_index(size_t) - { - return false; - } - bool add_primary_key(size_t) - { - return false; - } - bool remove_primary_key() - { - return false; - } bool set_link_type(ColKey) { return false; @@ -2094,67 +2057,27 @@ class NoOpTransactionLogParser { { return false; } - bool add_row_with_key(size_t, size_t, size_t, int64_t) - { - return false; - } bool remove_object(ObjKey) { return false; } - bool swap_rows(size_t, size_t) - { - return false; - } - bool move_row(size_t, size_t) - { - return false; - } - bool list_set(size_t) - { - return false; - } - bool list_clear(size_t) - { - return false; - } - bool list_erase(size_t) - { - return false; - } - bool link_list_nullify(size_t, size_t) - { - return false; - } - bool list_insert(size_t) - { - return false; - } - bool list_move(size_t, size_t) - { - return false; - } - bool dictionary_insert(size_t, Mixed) - { - return false; - } - bool dictionary_set(size_t, Mixed) + bool collection_set(size_t) { return false; } - bool dictionary_erase(size_t, Mixed) + bool collection_clear(size_t) { return false; } - bool set_insert(size_t) + bool collection_erase(size_t) { return false; } - bool set_erase(size_t) + bool collection_insert(size_t) { return false; } - bool set_clear(size_t) + bool collection_move(size_t, size_t) { return false; } @@ -2162,66 +2085,6 @@ class NoOpTransactionLogParser { { return false; } - bool add_int(size_t, size_t, int_fast64_t) - { - return false; - } - bool set_bool(size_t, size_t, bool, _impl::Instruction) - { - return false; - } - bool set_float(size_t, size_t, float, _impl::Instruction) - { - return false; - } - bool set_double(size_t, size_t, double, _impl::Instruction) - { - return false; - } - bool set_string(size_t, size_t, StringData, _impl::Instruction, size_t) - { - return false; - } - bool set_binary(size_t, size_t, BinaryData, _impl::Instruction) - { - return false; - } - bool set_timestamp(size_t, size_t, Timestamp, _impl::Instruction) - { - return false; - } - bool set_table(size_t, size_t, _impl::Instruction) - { - return false; - } - bool set_mixed(size_t, size_t, const Mixed&, _impl::Instruction) - { - return false; - } - bool set_link(size_t, size_t, size_t, size_t, _impl::Instruction) - { - return false; - } - bool set_null(size_t, size_t, _impl::Instruction, size_t) - { - return false; - } - bool nullify_link(size_t, size_t, size_t) - { - return false; - } - bool insert_substring(size_t, size_t, size_t, StringData) - { - return false; - } - bool erase_substring(size_t, size_t, size_t, size_t) - { - return false; - } - bool optimize_table() - { - return false; - } bool typed_link_change(ColKey, TableKey) { return true; @@ -2351,13 +2214,13 @@ TEST_TYPES(LangBindHelper_AdvanceReadTransact_TransactLog, AdvanceReadTransact, CHECK(o == o1 || o == o0); return true; } - bool select_link_list(ColKey col, ObjKey o) + bool select_collection(ColKey col, ObjKey o) { CHECK(col == link_list_col); CHECK(o == okey); return true; } - bool list_erase(size_t ndx) + bool collection_erase(size_t ndx) { CHECK(ndx == 0); return true; @@ -2399,7 +2262,7 @@ TEST_TYPES(LangBindHelper_AdvanceReadTransact_TransactLog, AdvanceReadTransact, struct : NoOpTransactionLogParser { using NoOpTransactionLogParser::NoOpTransactionLogParser; - bool list_clear(size_t old_size) const + bool collection_clear(size_t old_size) const { CHECK_EQUAL(3, old_size); return true; @@ -3042,109 +2905,6 @@ TEST(LangBindHelper_RollbackAndContinueAsRead_IntIndex) } -TEST(LangBindHelper_RollbackAndContinueAsRead_TransactLog) -{ - SHARED_GROUP_TEST_PATH(path); - std::unique_ptr hist(make_in_realm_history()); - DBRef sg = DB::create(*hist, path, DBOptions(crypt_key())); - ColKey c0, c1; - { - WriteTransaction wt(sg); - c0 = wt.add_table("table 1")->add_column(type_Int, "int"); - c1 = wt.add_table("table 2")->add_column(type_Int, "int"); - wt.commit(); - } - - auto tr = sg->start_read(); - - ObjKey o0, o1; - { - WriteTransaction wt(sg); - wt.get_table("table 1")->get_key(); - wt.get_table("table 2")->get_key(); - o0 = wt.get_table("table 1")->create_object().get_key(); - o1 = wt.get_table("table 2")->create_object().get_key(); - wt.commit(); - } - ColKey c2, c3; - ObjKey okey; - { - // Add a table with some links - WriteTransaction wt(sg); - TableRef table = wt.add_table("link origin"); - c2 = table->add_column(*wt.get_table("table 1"), "link"); - c3 = table->add_column_list(*wt.get_table("table 2"), "linklist"); - Obj o = table->create_object(); - o.set(c2, o.get_key()); - o.get_linklist(c3).add(o.get_key()); - okey = o.get_key(); - wt.commit(); - - tr->advance_read(); - } - { - // Verify that rolling back deletion restores links correctly - auto wt = sg->start_write(); - wt->get_table("table 1")->remove_object(o0); - wt->get_table("table 2")->remove_object(o1); - - struct : NoOpTransactionLogParser { - using NoOpTransactionLogParser::NoOpTransactionLogParser; - - bool create_object(ObjKey o) - { - CHECK(o == o1 || o == o0); - return true; - } - bool select_link_list(ColKey col, ObjKey o) - { - CHECK(col == link_list_col); - CHECK(o == okey); - return true; - } - bool list_insert(size_t ndx) - { - CHECK(ndx == 0); - return true; - } - - bool modify_object(ColKey col, ObjKey obj) - { - CHECK(col == link_col && obj == okey); - return true; - } - ObjKey o0, o1, okey; - ColKey link_col, link_list_col; - } parser(test_context); - parser.o1 = o1; - parser.o0 = o0; - parser.okey = okey; - parser.link_col = c2; - parser.link_list_col = c3; - wt->rollback_and_continue_as_read(&parser); - } - // Verify that clear() is rolled back appropriately - tr->promote_to_write(); - tr->get_table("link origin")->begin()->get_linklist(c3).clear(); - - { - struct foo : NoOpTransactionLogParser { - using NoOpTransactionLogParser::NoOpTransactionLogParser; - - size_t list_ndx = 0; - - bool list_insert(size_t ndx) - { - CHECK_EQUAL(list_ndx, ndx); - ++list_ndx; - return true; - } - } parser(test_context); - tr->rollback_and_continue_as_read(&parser); - CHECK(parser.list_ndx == 1); - } -} - TEST(LangBindHelper_ImplicitTransactions_OverSharedGroupDestruction) { SHARED_GROUP_TEST_PATH(path);