From eabaac367799aa1069dd7c2fdf87d63167b57316 Mon Sep 17 00:00:00 2001 From: Thomas Goyne Date: Tue, 5 Mar 2024 10:57:12 -0800 Subject: [PATCH] Make Obj trivial and add a separate ObjCollectionParent type Giving Obj a virtual base class turns out to have a meaningful performance impact. --- CHANGELOG.md | 1 + src/realm/collection.hpp | 8 +- src/realm/collection_parent.cpp | 239 ++++++----------------- src/realm/collection_parent.hpp | 26 ++- src/realm/dictionary.cpp | 7 +- src/realm/list.cpp | 12 +- src/realm/list.hpp | 2 +- src/realm/obj.cpp | 90 ++++++++- src/realm/obj.hpp | 128 ++++++++---- src/realm/path.hpp | 39 ++-- src/realm/replication.cpp | 2 +- src/realm/set.hpp | 2 +- src/realm/transaction.cpp | 2 +- test/object-store/dictionary.cpp | 2 - test/object-store/nested_collections.cpp | 1 - test/object-store/object.cpp | 1 - test/object-store/primitive_list.cpp | 1 - test/test_list.cpp | 110 +++++++++++ test/test_parser.cpp | 4 +- 19 files changed, 384 insertions(+), 293 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 80fe078951..4358e3d92f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ * Fixed an issue when removing items from a LnkLst that could result in invalidated links becoming visable which could cause crashes or exceptions when accessing those list items later on. This affects sync Realms where another client had previously removed a link in a linklist that has over 1000 links in it, and then further local removals from the same list caused the list to have fewer than 1000 items. ([#7414](https://github.com/realm/realm-core/pull/7414), since v10.0.0) * Query lists vs lists if the property to check is a path with wildcards would not give correct result. This has for a long time also been a problem for queries with linklist properties in the path ([#7393](https://github.com/realm/realm-core/issues/7393), since v14.0.0) * Fix a data race in change notification delivery when running at debug log level ([PR #7402](https://github.com/realm/realm-core/pull/7402), since v14.0.0). +* Fix a 10-15% performance regression when reading data from the Realm resulting from Obj being made a non-trivial type ([PR #7402](https://github.com/realm/realm-core/pull/7402), since v14.0.0). ### Breaking changes * The fix of querying involving multiple lists may cause tests that depended on the broken beharior to fail. diff --git a/src/realm/collection.hpp b/src/realm/collection.hpp index 7cb7481e51..78469a182a 100644 --- a/src/realm/collection.hpp +++ b/src/realm/collection.hpp @@ -571,7 +571,7 @@ class CollectionBaseImpl : public Interface, protected ArrayParent { using Interface::get_target_table; protected: - Obj m_obj_mem; + ObjCollectionParent m_obj_mem; std::shared_ptr m_col_parent; CollectionParent::Index m_index; ColKey m_col_key; @@ -724,19 +724,19 @@ class CollectionBaseImpl : public Interface, protected ArrayParent { void set_backlink(ColKey col_key, ObjLink new_link) const { check_parent(); - m_parent->set_backlink(col_key, new_link); + m_parent->get_object().set_backlink(col_key, new_link); } // Used when replacing a link, return true if CascadeState contains objects to remove bool replace_backlink(ColKey col_key, ObjLink old_link, ObjLink new_link, CascadeState& state) const { check_parent(); - return m_parent->replace_backlink(col_key, old_link, new_link, state); + return m_parent->get_object().replace_backlink(col_key, old_link, new_link, state); } // Used when removing a backlink, return true if CascadeState contains objects to remove bool remove_backlink(ColKey col_key, ObjLink old_link, CascadeState& state) const { check_parent(); - return m_parent->remove_backlink(col_key, old_link, state); + return m_parent->get_object().remove_backlink(col_key, old_link, state); } /// Reset the accessor's tracking of the content version. Derived classes diff --git a/src/realm/collection_parent.cpp b/src/realm/collection_parent.cpp index f852c7575c..dc3ad806bc 100644 --- a/src/realm/collection_parent.cpp +++ b/src/realm/collection_parent.cpp @@ -79,212 +79,81 @@ void CollectionParent::check_level() const throw LogicError(ErrorCodes::LimitExceeded, "Max nesting level reached"); } } -void CollectionParent::set_backlink(ColKey col_key, ObjLink new_link) const -{ - if (new_link && new_link.get_obj_key()) { - auto t = get_table(); - auto target_table = t->get_parent_group()->get_table(new_link.get_table_key()); - ColKey backlink_col_key; - auto type = col_key.get_type(); - if (type == col_type_TypedLink || type == col_type_Mixed || col_key.is_dictionary()) { - // This may modify the target table - backlink_col_key = target_table->find_or_add_backlink_column(col_key, t->get_key()); - // it is possible that this was a link to the same table and that adding a backlink column has - // caused the need to update this object as well. - update_if_needed(); - } - else { - backlink_col_key = t->get_opposite_column(col_key); - } - auto obj_key = new_link.get_obj_key(); - auto target_obj = obj_key.is_unresolved() ? target_table->try_get_tombstone(obj_key) - : target_table->try_get_object(obj_key); - if (!target_obj) { - throw InvalidArgument(ErrorCodes::KeyNotFound, "Target object not found"); - } - target_obj.add_backlink(backlink_col_key, get_object().get_key()); - } -} -bool CollectionParent::replace_backlink(ColKey col_key, ObjLink old_link, ObjLink new_link, CascadeState& state) const +template typename Collection, typename LinkCol> +std::unique_ptr create_collection(ColKey col_key, size_t level) { - bool recurse = remove_backlink(col_key, old_link, state); - set_backlink(col_key, new_link); - - return recurse; -} - -bool CollectionParent::remove_backlink(ColKey col_key, ObjLink old_link, CascadeState& state) const -{ - if (old_link && old_link.get_obj_key()) { - auto t = get_table(); - REALM_ASSERT(t->valid_column(col_key)); - ObjKey old_key = old_link.get_obj_key(); - auto target_obj = t->get_parent_group()->get_object(old_link); - TableRef target_table = target_obj.get_table(); - ColKey backlink_col_key; - auto type = col_key.get_type(); - if (type == col_type_TypedLink || type == col_type_Mixed || col_key.is_dictionary()) { - backlink_col_key = target_table->find_or_add_backlink_column(col_key, t->get_key()); - } - else { - backlink_col_key = t->get_opposite_column(col_key); - } - - bool strong_links = target_table->is_embedded(); - bool is_unres = old_key.is_unresolved(); - - bool last_removed = target_obj.remove_one_backlink(backlink_col_key, get_object().get_key()); // Throws - if (is_unres) { - if (last_removed) { - // Check is there are more backlinks - if (!target_obj.has_backlinks(false)) { - // Tombstones can be erased right away - there is no cascading effect - target_table->m_tombstones->erase(old_key, state); - } - } - } - else { - return state.enqueue_for_cascade(target_obj, strong_links, last_removed); - } - } - - return false; -} - -LstBasePtr CollectionParent::get_listbase_ptr(ColKey col_key) const -{ - auto table = get_table(); - auto attr = table->get_column_attr(col_key); - REALM_ASSERT(attr.test(col_attr_List) || attr.test(col_attr_Nullable)); - bool nullable = attr.test(col_attr_Nullable); - - switch (table->get_column_type(col_key)) { - case type_Int: { + bool nullable = col_key.get_attrs().test(col_attr_Nullable); + switch (col_key.get_type()) { + case col_type_Int: if (nullable) - return std::make_unique>>(col_key); - else - return std::make_unique>(col_key); - } - case type_Bool: { + return std::make_unique>>(col_key); + return std::make_unique>(col_key); + case col_type_Bool: if (nullable) - return std::make_unique>>(col_key); - else - return std::make_unique>(col_key); - } - case type_Float: { + return std::make_unique>>(col_key); + return std::make_unique>(col_key); + case col_type_Float: if (nullable) - return std::make_unique>>(col_key); - else - return std::make_unique>(col_key); - } - case type_Double: { + return std::make_unique>>(col_key); + return std::make_unique>(col_key); + case col_type_Double: if (nullable) - return std::make_unique>>(col_key); - else - return std::make_unique>(col_key); - } - case type_String: { - return std::make_unique>(col_key); - } - case type_Binary: { - return std::make_unique>(col_key); - } - case type_Timestamp: { - return std::make_unique>(col_key); - } - case type_Decimal: { - return std::make_unique>(col_key); - } - case type_ObjectId: { + return std::make_unique>>(col_key); + return std::make_unique>(col_key); + case col_type_String: + return std::make_unique>(col_key); + case col_type_Binary: + return std::make_unique>(col_key); + case col_type_Timestamp: + return std::make_unique>(col_key); + case col_type_Decimal: + return std::make_unique>(col_key); + case col_type_ObjectId: if (nullable) - return std::make_unique>>(col_key); - else - return std::make_unique>(col_key); - } - case type_UUID: { + return std::make_unique>>(col_key); + return std::make_unique>(col_key); + case col_type_UUID: if (nullable) - return std::make_unique>>(col_key); - else - return std::make_unique>(col_key); - } - case type_TypedLink: { - return std::make_unique>(col_key); - } - case type_Mixed: { - return std::make_unique>(col_key, get_level() + 1); - } - case type_Link: - return std::make_unique(col_key); + return std::make_unique>>(col_key); + return std::make_unique>(col_key); + case col_type_TypedLink: + return std::make_unique>(col_key); + case col_type_Mixed: + return std::make_unique>(col_key, level + 1); + case col_type_Link: + return std::make_unique(col_key); + default: + REALM_TERMINATE("Unsupported column type."); } - REALM_TERMINATE("Unsupported column type"); } -SetBasePtr CollectionParent::get_setbase_ptr(ColKey col_key) const +LstBasePtr CollectionParent::get_listbase_ptr(ColKey col_key, size_t level) { - auto table = get_table(); - auto attr = table->get_column_attr(col_key); - REALM_ASSERT(attr.test(col_attr_Set)); - bool nullable = attr.test(col_attr_Nullable); + REALM_ASSERT(col_key.get_attrs().test(col_attr_List) || col_key.get_type() == col_type_Mixed); + return create_collection(col_key, level); +} - switch (table->get_column_type(col_key)) { - case type_Int: - if (nullable) - return std::make_unique>>(col_key); - return std::make_unique>(col_key); - case type_Bool: - if (nullable) - return std::make_unique>>(col_key); - return std::make_unique>(col_key); - case type_Float: - if (nullable) - return std::make_unique>>(col_key); - return std::make_unique>(col_key); - case type_Double: - if (nullable) - return std::make_unique>>(col_key); - return std::make_unique>(col_key); - case type_String: - return std::make_unique>(col_key); - case type_Binary: - return std::make_unique>(col_key); - case type_Timestamp: - return std::make_unique>(col_key); - case type_Decimal: - return std::make_unique>(col_key); - case type_ObjectId: - if (nullable) - return std::make_unique>>(col_key); - return std::make_unique>(col_key); - case type_UUID: - if (nullable) - return std::make_unique>>(col_key); - return std::make_unique>(col_key); - case type_TypedLink: - return std::make_unique>(col_key); - case type_Mixed: - return std::make_unique>(col_key); - case type_Link: - return std::make_unique(col_key); - } - REALM_TERMINATE("Unsupported column type."); +SetBasePtr CollectionParent::get_setbase_ptr(ColKey col_key, size_t level) +{ + REALM_ASSERT(col_key.get_attrs().test(col_attr_Set)); + return create_collection(col_key, level); } -CollectionBasePtr CollectionParent::get_collection_ptr(ColKey col_key) const +CollectionBasePtr CollectionParent::get_collection_ptr(ColKey col_key, size_t level) { if (col_key.is_list()) { - return get_listbase_ptr(col_key); + return get_listbase_ptr(col_key, level); } else if (col_key.is_set()) { - return get_setbase_ptr(col_key); + return get_setbase_ptr(col_key, level); } else if (col_key.is_dictionary()) { - return std::make_unique(col_key, get_level() + 1); + return std::make_unique(col_key, level + 1); } return {}; } - int64_t CollectionParent::generate_key(size_t sz) { static std::mt19937 gen32; @@ -307,5 +176,13 @@ int64_t CollectionParent::generate_key(size_t sz) return key; } +void CollectionParent::set_key(BPlusTreeMixed& tree, size_t index) +{ + int64_t key = generate_key(tree.size()); + while (tree.find_key(key) != realm::not_found) { + key++; + } + tree.set_key(index, key); +} } // namespace realm diff --git a/src/realm/collection_parent.hpp b/src/realm/collection_parent.hpp index 29ffbe3a70..53fe3ecb71 100644 --- a/src/realm/collection_parent.hpp +++ b/src/realm/collection_parent.hpp @@ -20,15 +20,16 @@ #define REALM_COLLECTION_PARENT_HPP #include -#include -#include #include +#include +#include namespace realm { class Obj; class Replication; class CascadeState; +class BPlusTreeMixed; class Collection; class CollectionBase; @@ -95,6 +96,13 @@ class CollectionParent : public std::enable_shared_from_this { /// Get table of owning object virtual TableRef get_table() const noexcept = 0; + static LstBasePtr get_listbase_ptr(ColKey col_key, size_t level); + static SetBasePtr get_setbase_ptr(ColKey col_key, size_t level); + static CollectionBasePtr get_collection_ptr(ColKey col_key, size_t level); + + static int64_t generate_key(size_t sz); + static void set_key(BPlusTreeMixed& tree, size_t index); + protected: friend class Collection; template @@ -129,20 +137,8 @@ class CollectionParent : public std::enable_shared_from_this { /// Set the top ref in parent virtual void set_collection_ref(Index, ref_type ref, CollectionType) = 0; + /// Get the counter which is incremented whenever the root Obj is updated. virtual uint32_t parent_version() const noexcept = 0; - - // Used when inserting a new link. You will not remove existing links in this process - void set_backlink(ColKey col_key, ObjLink new_link) const; - // Used when replacing a link, return true if CascadeState contains objects to remove - bool replace_backlink(ColKey col_key, ObjLink old_link, ObjLink new_link, CascadeState& state) const; - // Used when removing a backlink, return true if CascadeState contains objects to remove - bool remove_backlink(ColKey col_key, ObjLink old_link, CascadeState& state) const; - - LstBasePtr get_listbase_ptr(ColKey col_key) const; - SetBasePtr get_setbase_ptr(ColKey col_key) const; - CollectionBasePtr get_collection_ptr(ColKey col_key) const; - - static int64_t generate_key(size_t sz); }; } // namespace realm diff --git a/src/realm/dictionary.cpp b/src/realm/dictionary.cpp index 561d63e325..b4e5ac67d1 100644 --- a/src/realm/dictionary.cpp +++ b/src/realm/dictionary.cpp @@ -441,11 +441,7 @@ void Dictionary::insert_collection(const PathElement& path_elem, CollectionType if (!old_val || *old_val != new_val) { m_values->ensure_keys(); auto [it, inserted] = insert(path_elem.get_key(), new_val); - int64_t key = generate_key(size()); - while (m_values->find_key(key) != realm::not_found) { - key++; - } - m_values->set_key(it.index(), key); + set_key(*m_values, it.index()); } } @@ -682,7 +678,6 @@ void Dictionary::ensure_created() { constexpr bool allow_create = true; if (do_update_if_needed(allow_create) == UpdateStatus::Detached) { - // FIXME: untested throw StaleAccessor("Dictionary no longer exists"); } } diff --git a/src/realm/list.cpp b/src/realm/list.cpp index d51adacd94..5a5482b70d 100644 --- a/src/realm/list.cpp +++ b/src/realm/list.cpp @@ -534,11 +534,7 @@ void Lst::insert_collection(const PathElement& path_elem, CollectionType check_level(); m_tree->ensure_keys(); insert(path_elem.get_ndx(), Mixed(0, dict_or_list)); - int64_t key = generate_key(size()); - while (m_tree->find_key(key) != realm::not_found) { - key++; - } - m_tree->set_key(path_elem.get_ndx(), key); + set_key(*m_tree, path_elem.get_ndx()); bump_content_version(); } @@ -560,11 +556,7 @@ void Lst::set_collection(const PathElement& path_elem, CollectionType dic set(ndx, new_val); int64_t key = m_tree->get_key(ndx); if (key == 0) { - key = generate_key(size()); - while (m_tree->find_key(key) != realm::not_found) { - key++; - } - m_tree->set_key(ndx, key); + set_key(*m_tree, path_elem.get_ndx()); } bump_content_version(); } diff --git a/src/realm/list.hpp b/src/realm/list.hpp index 945b3d53fd..fec6505c03 100644 --- a/src/realm/list.hpp +++ b/src/realm/list.hpp @@ -629,7 +629,7 @@ class LnkLst final : public ObjCollectionBase { } void add(const Obj& obj) { - if (get_target_table()->get_key() != obj.get_table_key()) { + if (get_target_table() != obj.get_table()) { throw InvalidArgument("LnkLst::add: Wrong object type"); } add(obj.get_key()); diff --git a/src/realm/obj.cpp b/src/realm/obj.cpp index 2a9a53c629..afabea7a08 100644 --- a/src/realm/obj.cpp +++ b/src/realm/obj.cpp @@ -1107,7 +1107,7 @@ StablePath Obj::get_stable_path() const noexcept return {}; } -void Obj::add_index(Path& path, const Index& index) const +void Obj::add_index(Path& path, const CollectionParent::Index& index) const { if (path.empty()) { path.emplace_back(get_table()->get_column_key(index)); @@ -1965,14 +1965,14 @@ void Obj::handle_multiple_backlinks_during_schema_migration() LstBasePtr Obj::get_listbase_ptr(ColKey col_key) const { - auto list = CollectionParent::get_listbase_ptr(col_key); + auto list = CollectionParent::get_listbase_ptr(col_key, 0); list->set_owner(*this, col_key); return list; } SetBasePtr Obj::get_setbase_ptr(ColKey col_key) const { - auto set = CollectionParent::get_setbase_ptr(col_key); + auto set = CollectionParent::get_setbase_ptr(col_key, 0); set->set_owner(*this, col_key); return set; } @@ -2031,7 +2031,7 @@ Obj& Obj::set_collection(ColKey col_key, CollectionType type) values.init_from_parent(); values.set(m_row_ndx, new_val); - values.set_key(m_row_ndx, generate_key(0x10)); + values.set_key(m_row_ndx, CollectionParent::generate_key(0x10)); sync(fields); @@ -2139,7 +2139,7 @@ CollectionPtr Obj::get_collection_by_stable_path(const StablePath& path) const CollectionBasePtr Obj::get_collection_ptr(ColKey col_key) const { if (col_key.is_collection()) { - auto collection = CollectionParent::get_collection_ptr(col_key); + auto collection = CollectionParent::get_collection_ptr(col_key, 0); collection->set_owner(*this, col_key); return collection; } @@ -2439,7 +2439,7 @@ ref_type Obj::Internal::get_ref(const Obj& obj, ColKey col_key) return to_ref(obj._get(col_key.get_index())); } -ref_type Obj::get_collection_ref(Index index, CollectionType type) const +ref_type Obj::get_collection_ref(StableIndex index, CollectionType type) const { if (index.is_collection()) { return to_ref(_get(index.get_index())); @@ -2454,7 +2454,7 @@ ref_type Obj::get_collection_ref(Index index, CollectionType type) const throw StaleAccessor("This collection is no more"); } -bool Obj::check_collection_ref(Index index, CollectionType type) const noexcept +bool Obj::check_collection_ref(StableIndex index, CollectionType type) const noexcept { if (index.is_collection()) { return true; @@ -2465,7 +2465,7 @@ bool Obj::check_collection_ref(Index index, CollectionType type) const noexcept return false; } -void Obj::set_collection_ref(Index index, ref_type ref, CollectionType type) +void Obj::set_collection_ref(StableIndex index, ref_type ref, CollectionType type) { if (index.is_collection()) { set_int(index.get_index(), from_ref(ref)); @@ -2474,4 +2474,78 @@ void Obj::set_collection_ref(Index index, ref_type ref, CollectionType type) set_ref(index.get_index(), ref, type); } +void Obj::set_backlink(ColKey col_key, ObjLink new_link) const +{ + if (!new_link) { + return; + } + + auto target_table = m_table->get_parent_group()->get_table(new_link.get_table_key()); + ColKey backlink_col_key; + auto type = col_key.get_type(); + if (type == col_type_TypedLink || type == col_type_Mixed || col_key.is_dictionary()) { + // This may modify the target table + backlink_col_key = target_table->find_or_add_backlink_column(col_key, m_table->get_key()); + // it is possible that this was a link to the same table and that adding a backlink column has + // caused the need to update this object as well. + update_if_needed(); + } + else { + backlink_col_key = m_table->get_opposite_column(col_key); + } + auto obj_key = new_link.get_obj_key(); + auto target_obj = + obj_key.is_unresolved() ? target_table->try_get_tombstone(obj_key) : target_table->try_get_object(obj_key); + if (!target_obj) { + throw InvalidArgument(ErrorCodes::KeyNotFound, "Target object not found"); + } + target_obj.add_backlink(backlink_col_key, m_key); +} + +bool Obj::replace_backlink(ColKey col_key, ObjLink old_link, ObjLink new_link, CascadeState& state) const +{ + bool recurse = remove_backlink(col_key, old_link, state); + set_backlink(col_key, new_link); + return recurse; +} + +bool Obj::remove_backlink(ColKey col_key, ObjLink old_link, CascadeState& state) const +{ + if (!old_link) { + return false; + } + + REALM_ASSERT(m_table->valid_column(col_key)); + ObjKey old_key = old_link.get_obj_key(); + auto target_obj = m_table->get_parent_group()->get_object(old_link); + TableRef target_table = target_obj.get_table(); + ColKey backlink_col_key; + auto type = col_key.get_type(); + if (type == col_type_TypedLink || type == col_type_Mixed || col_key.is_dictionary()) { + backlink_col_key = target_table->find_or_add_backlink_column(col_key, m_table->get_key()); + } + else { + backlink_col_key = m_table->get_opposite_column(col_key); + } + + bool strong_links = target_table->is_embedded(); + bool is_unres = old_key.is_unresolved(); + + bool last_removed = target_obj.remove_one_backlink(backlink_col_key, m_key); // Throws + if (is_unres) { + if (last_removed) { + // Check is there are more backlinks + if (!target_obj.has_backlinks(false)) { + // Tombstones can be erased right away - there is no cascading effect + target_table->m_tombstones->erase(old_key, state); + } + } + } + else { + return state.enqueue_for_cascade(target_obj, strong_links, last_removed); + } + + return false; +} + } // namespace realm diff --git a/src/realm/obj.hpp b/src/realm/obj.hpp index cdb59f6f5f..8d2d4938ef 100644 --- a/src/realm/obj.hpp +++ b/src/realm/obj.hpp @@ -57,42 +57,30 @@ class DeepChangeChecker; } // 'Object' would have been a better name, but it clashes with a class in ObjectStore -class Obj : public CollectionParent { +class Obj { public: constexpr Obj() = default; Obj(TableRef table, MemRef mem, ObjKey key, size_t row_ndx); - // Overriding members of CollectionParent: - UpdateStatus update_if_needed() const final; + // CollectionParent implementation + UpdateStatus update_if_needed() const; // Get the path in a minimal format without including object accessors. // If you need to obtain additional information for each object in the path, // you should use get_fat_path() or traverse_path() instead (see below). - FullPath get_path() const final; + FullPath get_path() const; std::string get_id() const; - Path get_short_path() const noexcept final; - ColKey get_col_key() const noexcept final; - StablePath get_stable_path() const noexcept final; - void add_index(Path& path, const Index& ndx) const final; - size_t find_index(const Index&) const final - { - return realm::npos; - } + Path get_short_path() const noexcept; + ColKey get_col_key() const noexcept; + StablePath get_stable_path() const noexcept; + void add_index(Path& path, const CollectionParent::Index& ndx) const; - TableRef get_table() const noexcept final + TableRef get_table() const noexcept { return m_table.cast_away_const(); } - const Obj& get_object() const noexcept final - { - return *this; - } - ref_type get_collection_ref(Index, CollectionType) const final; - bool check_collection_ref(Index, CollectionType) const noexcept final; - void set_collection_ref(Index, ref_type, CollectionType) final; - uint32_t parent_version() const noexcept final - { - return m_version_counter; - } + ref_type get_collection_ref(CollectionParent::Index, CollectionType) const; + bool check_collection_ref(CollectionParent::Index, CollectionType) const noexcept; + void set_collection_ref(CollectionParent::Index, ref_type, CollectionType); StableIndex build_index(ColKey) const; bool check_index(StableIndex) const; @@ -322,22 +310,18 @@ class Obj : public CollectionParent { friend class ArrayBacklink; friend class CascadeState; friend class Cluster; + friend class CollectionParent; friend class ColumnListBase; - friend class CollectionBase; + friend class LinkCount; + friend class LinkMap; + friend class Lst; + friend class ObjCollectionParent; + friend class Table; friend class TableView; template friend class CollectionBaseImpl; template - friend class Lst; - friend class LnkLst; - friend class LinkCount; - friend class Dictionary; - friend class LinkMap; - template friend class Set; - friend class Table; - friend class Transaction; - friend class CollectionParent; mutable TableRef m_table; ObjKey m_key; @@ -417,6 +401,82 @@ class Obj : public CollectionParent { bool compare_values(Mixed, Mixed, ColKey, Obj, StringData) const; bool compare_list_in_mixed(Lst&, Lst&, ColKey, Obj, StringData) const; bool compare_dict_in_mixed(Dictionary&, Dictionary&, ColKey, Obj, StringData) const; + + // Used when inserting a new link. You will not remove existing links in this process + void set_backlink(ColKey col_key, ObjLink new_link) const; + // Used when replacing a link, return true if CascadeState contains objects to remove + bool replace_backlink(ColKey col_key, ObjLink old_link, ObjLink new_link, CascadeState& state) const; + // Used when removing a backlink, return true if CascadeState contains objects to remove + bool remove_backlink(ColKey col_key, ObjLink old_link, CascadeState& state) const; +}; +static_assert(std::is_trivially_destructible_v); + +class ObjCollectionParent : public Obj, public CollectionParent { +public: + ObjCollectionParent() = default; + ObjCollectionParent(const Obj& obj) noexcept + : Obj(obj) + { + } + ObjCollectionParent& operator=(const Obj& obj) noexcept + { + static_cast(*this) = obj; + return *this; + } + +private: + FullPath get_path() const override + { + return Obj::get_path(); + } + Path get_short_path() const override + { + return Obj::get_short_path(); + } + ColKey get_col_key() const noexcept override + { + return Obj::get_col_key(); + } + StablePath get_stable_path() const override + { + return Obj::get_stable_path(); + } + void add_index(Path& path, const Index& ndx) const override + { + Obj::add_index(path, ndx); + } + size_t find_index(const Index&) const override + { + return realm::npos; + } + TableRef get_table() const noexcept override + { + return Obj::get_table(); + } + UpdateStatus update_if_needed() const override + { + return Obj::update_if_needed(); + } + const Obj& get_object() const noexcept override + { + return *this; + } + uint32_t parent_version() const noexcept override + { + return m_version_counter; + } + ref_type get_collection_ref(Index index, CollectionType type) const override + { + return Obj::get_collection_ref(index, type); + } + bool check_collection_ref(Index index, CollectionType type) const noexcept override + { + return Obj::check_collection_ref(index, type); + } + void set_collection_ref(Index index, ref_type ref, CollectionType type) override + { + Obj::set_collection_ref(index, ref, type); + } }; std::ostream& operator<<(std::ostream&, const Obj& obj); diff --git a/src/realm/path.hpp b/src/realm/path.hpp index 08ead2dbae..6124590271 100644 --- a/src/realm/path.hpp +++ b/src/realm/path.hpp @@ -271,54 +271,45 @@ class ExtendedColumnKey { */ class StableIndex { public: - StableIndex() - { - value.raw = 0; - } + StableIndex() = default; StableIndex(ColKey col_key, int64_t salt) { - value.col_index = col_key.get_index().val; - value.is_collection = col_key.is_collection(); - value.is_column = true; - value.salt = int32_t(salt); + m_col_index = col_key.get_index().val; + m_is_collection = col_key.is_collection(); + m_is_column = true; + m_salt = int32_t(salt); } StableIndex(int64_t salt) { - value.raw = 0; - value.salt = int32_t(salt); + m_salt = int32_t(salt); } int64_t get_salt() const { - return value.salt; + return m_salt; } ColKey::Idx get_index() const noexcept { - return {unsigned(value.col_index)}; + return {unsigned(m_col_index)}; } bool is_collection() const noexcept { - return value.is_collection; + return m_is_collection; } bool operator==(const StableIndex& other) const noexcept { - return value.is_column ? value.col_index == other.value.col_index : value.salt == other.value.salt; + return m_is_column ? m_col_index == other.m_col_index : m_salt == other.m_salt; } bool operator<(const StableIndex& other) const noexcept { - return value.is_column ? value.col_index < other.value.col_index : value.salt < other.value.salt; + return m_is_column ? m_col_index < other.m_col_index : m_salt < other.m_salt; } private: - union { - struct { - bool is_column; - bool is_collection; - int16_t col_index; - int32_t salt; - }; - int64_t raw; - } value; + bool m_is_column = false; + bool m_is_collection = false; + int16_t m_col_index = 0; + int32_t m_salt = 0; }; static_assert(sizeof(StableIndex) == 8); diff --git a/src/realm/replication.cpp b/src/realm/replication.cpp index 35bd32d8b3..cfc60c6580 100644 --- a/src/realm/replication.cpp +++ b/src/realm/replication.cpp @@ -177,7 +177,7 @@ void Replication::remove_object(const Table* t, ObjKey key) m_encoder.remove_object(key); // Throws } -inline void Replication::select_obj(ObjKey key) +void Replication::select_obj(ObjKey key) { if (key == m_selected_obj) { return; diff --git a/src/realm/set.hpp b/src/realm/set.hpp index 2fe0a762f5..e3d7fac3d6 100644 --- a/src/realm/set.hpp +++ b/src/realm/set.hpp @@ -103,7 +103,7 @@ class Set final : public CollectionBaseImpl { this->set_owner(owner, col_key); } - Set(ColKey col_key) + Set(ColKey col_key, size_t = 0) : Base(col_key) { if (!col_key.is_set()) { diff --git a/src/realm/transaction.cpp b/src/realm/transaction.cpp index 6f246e36e2..1cddd8fdce 100644 --- a/src/realm/transaction.cpp +++ b/src/realm/transaction.cpp @@ -419,7 +419,7 @@ _impl::History* Transaction::get_history() const Obj Transaction::import_copy_of(const Obj& original) { if (bool(original) && original.is_valid()) { - TableKey tk = original.get_table_key(); + TableKey tk = original.get_table()->get_key(); ObjKey rk = original.get_key(); auto table = get_table(tk); if (table->is_valid(rk)) diff --git a/test/object-store/dictionary.cpp b/test/object-store/dictionary.cpp index f72791b6ed..0ae1839684 100644 --- a/test/object-store/dictionary.cpp +++ b/test/object-store/dictionary.cpp @@ -63,9 +63,7 @@ struct StringMaker { namespace cf = realm::collection_fixtures; TEST_CASE("nested dictionary in mixed", "[dictionary]") { - InMemoryTestFile config; - config.cache = false; config.automatic_change_notifications = false; config.schema = Schema{{"any_collection", {{"any", PropertyType::Mixed | PropertyType::Nullable}}}}; diff --git a/test/object-store/nested_collections.cpp b/test/object-store/nested_collections.cpp index cd94a94b07..99962d2533 100644 --- a/test/object-store/nested_collections.cpp +++ b/test/object-store/nested_collections.cpp @@ -36,7 +36,6 @@ using namespace realm; TEST_CASE("nested-list-mixed", "[nested-collections]") { InMemoryTestFile config; - config.cache = false; config.automatic_change_notifications = false; auto r = Realm::get_shared_realm(config); r->update_schema({{ diff --git a/test/object-store/object.cpp b/test/object-store/object.cpp index b370234616..ea77920172 100644 --- a/test/object-store/object.cpp +++ b/test/object-store/object.cpp @@ -160,7 +160,6 @@ TEST_CASE("object") { _impl::RealmCoordinator::assert_no_open_realms(); InMemoryTestFile config; - config.cache = false; config.automatic_change_notifications = false; config.schema_mode = SchemaMode::AdditiveExplicit; config.schema = Schema{ diff --git a/test/object-store/primitive_list.cpp b/test/object-store/primitive_list.cpp index 3160c88cc3..9bab1b1dee 100644 --- a/test/object-store/primitive_list.cpp +++ b/test/object-store/primitive_list.cpp @@ -983,7 +983,6 @@ TEST_CASE("list of mixed links", "[primitives]") { TEST_CASE("list of strings - with index", "[primitives]") { InMemoryTestFile config; - config.cache = false; config.automatic_change_notifications = false; config.schema = Schema{ {"object", diff --git a/test/test_list.cpp b/test/test_list.cpp index 5ff8d77640..dbc786f7d3 100644 --- a/test/test_list.cpp +++ b/test/test_list.cpp @@ -1070,3 +1070,113 @@ TEST(List_Nested_Replication) Dictionary dict3(*dict, dict2_index); CHECK_EQUAL(dict3.get_col_key(), col_any); } + +namespace realm { +static std::ostream& operator<<(std::ostream& os, UpdateStatus status) +{ + switch (status) { + case UpdateStatus::Detached: + os << "Detatched"; + break; + case UpdateStatus::Updated: + os << "Updated"; + break; + case UpdateStatus::NoChange: + os << "NoChange"; + break; + } + return os; +} +} // namespace realm + +TEST(List_UpdateIfNeeded) +{ + SHARED_GROUP_TEST_PATH(path); + DBRef db = DB::create(make_in_realm_history(), path); + auto tr = db->start_write(); + auto table = tr->add_table("table"); + auto col = table->add_column(type_Mixed, "mixed"); + auto col2 = table->add_column(type_Mixed, "col2"); + table->create_object(); + Obj obj = table->create_object(); + obj.set_collection(col, CollectionType::List); + + auto list_1 = obj.get_list(col); + auto list_2 = obj.get_list(col); + + // The underlying object starts out up-to-date + CHECK_EQUAL(list_1.get_obj().update_if_needed(), UpdateStatus::NoChange); + + // Attempt to initialize the accessor and fail because the list is empty, + // leaving it detached (only size() can be called on an empty list) + CHECK_EQUAL(list_1.update_if_needed(), UpdateStatus::Detached); + CHECK_EQUAL(list_2.update_if_needed(), UpdateStatus::Detached); + + list_1.add(Mixed()); + + // First accessor was used to create the list so it's already up to date, + // but the second is updated + CHECK_EQUAL(list_1.update_if_needed(), UpdateStatus::NoChange); + CHECK_EQUAL(list_2.update_if_needed(), UpdateStatus::Updated); + + // The list is now non-empty, so a new accessor can initialize + auto list_3 = obj.get_list(col); + CHECK_EQUAL(list_3.update_if_needed(), UpdateStatus::Updated); + + // Update the row index of the parent object, forcing it to update + table->remove_object(table->begin()); + + // Updating the base object directly first doesn't change the result of + // updating the list + CHECK_EQUAL(list_1.get_obj().update_if_needed(), UpdateStatus::Updated); + CHECK_EQUAL(list_1.update_if_needed(), UpdateStatus::Updated); + + CHECK_EQUAL(list_2.update_if_needed(), UpdateStatus::Updated); + CHECK_EQUAL(list_3.update_if_needed(), UpdateStatus::Updated); + + tr->commit_and_continue_as_read(); + + // Committing the write transaction changes the obj's ref, so everything + // has to update + CHECK_EQUAL(list_1.get_obj().update_if_needed(), UpdateStatus::Updated); + CHECK_EQUAL(list_1.update_if_needed(), UpdateStatus::Updated); + CHECK_EQUAL(list_2.update_if_needed(), UpdateStatus::Updated); + CHECK_EQUAL(list_3.update_if_needed(), UpdateStatus::Updated); + + // Perform a write which does not result in obj changing + { + auto tr2 = db->start_write(); + tr2->add_table("other table"); + tr2->commit(); + } + tr->advance_read(); + + // The obj's storage version has changed, but nothing needs to update + CHECK_EQUAL(list_1.get_obj().update_if_needed(), UpdateStatus::NoChange); + CHECK_EQUAL(list_1.update_if_needed(), UpdateStatus::NoChange); + CHECK_EQUAL(list_2.update_if_needed(), UpdateStatus::NoChange); + CHECK_EQUAL(list_3.update_if_needed(), UpdateStatus::NoChange); + + // Perform a write which does modify obj + { + auto tr2 = db->start_write(); + tr2->get_table("table")->get_object(obj.get_key()).set_any(col2, "value"); + tr2->commit(); + } + tr->advance_read(); + + // Everything needs to update even though the allocator's content version is unchanged + CHECK_EQUAL(list_1.get_obj().update_if_needed(), UpdateStatus::Updated); + CHECK_EQUAL(list_1.update_if_needed(), UpdateStatus::Updated); + CHECK_EQUAL(list_2.update_if_needed(), UpdateStatus::Updated); + CHECK_EQUAL(list_3.update_if_needed(), UpdateStatus::Updated); + + // Everything updates to detached when the object is removed + tr->promote_to_write(); + obj.remove(); + + CHECK_EQUAL(list_1.get_obj().update_if_needed(), UpdateStatus::Detached); + CHECK_EQUAL(list_1.update_if_needed(), UpdateStatus::Detached); + CHECK_EQUAL(list_2.update_if_needed(), UpdateStatus::Detached); + CHECK_EQUAL(list_3.update_if_needed(), UpdateStatus::Detached); +} diff --git a/test/test_parser.cpp b/test/test_parser.cpp index 2fd69da5a6..d60d5c1d21 100644 --- a/test/test_parser.cpp +++ b/test/test_parser.cpp @@ -5843,8 +5843,8 @@ TEST(Parser_CollectionLinks) Obj charlie = persons->create_object_with_primary_key("charlie"); Obj david = persons->create_object_with_primary_key("david"); - Obj elisabeth = persons->create_object_with_primary_key("elisabeth"); - Obj felix = persons->create_object_with_primary_key("felix"); + persons->create_object_with_primary_key("elisabeth"); + persons->create_object_with_primary_key("felix"); Obj gary = persons->create_object_with_primary_key("gary"); Obj hutch = persons->create_object_with_primary_key("hutch");