Consolidate collection updating logic somewhat

Author: tgoyne

src/realm/collection.cpp

@@ -237,4 +237,25 @@ void Collection::get_any(QueryCtrlBlock& ctrl, Mixed val, size_t index)
     }
 }
 
+UpdateStatus CollectionBase::do_init_from_parent(BPlusTreeBase* tree, ref_type ref, bool allow_create)
+{
+    if (ref) {
+        tree->init_from_ref(ref);
+    }
+    else {
+        if (tree->init_from_parent()) {
+            // All is well
+            return UpdateStatus::Updated;
+        }
+        if (!allow_create) {
+            tree->detach();
+            return UpdateStatus::Detached;
+        }
+        // The ref in the column was NULL, create the tree in place.
+        tree->create();
+        REALM_ASSERT(tree->is_attached());
+    }
+    return UpdateStatus::Updated;
+}
+
 } // namespace realm

src/realm/collection.hpp

@@ -53,18 +53,18 @@ class DummyParent : public CollectionParent {
     {
         return m_obj;
     }
+    uint32_t parent_version() const noexcept final
+    {
+        return 0;
+    }
 
 protected:
     Obj m_obj;
     ref_type m_ref;
-    UpdateStatus update_if_needed_with_status() const final
+    UpdateStatus update_if_needed() const final
     {
         return UpdateStatus::Updated;
     }
-    bool update_if_needed() const final
-    {
-        return true;
-    }
     ref_type get_collection_ref(Index, CollectionType) const final
     {
         return m_ref;
@@ -255,6 +255,7 @@ class CollectionBase : public Collection {
     CollectionBase& operator=(CollectionBase&&) noexcept = default;
 
     void validate_index(const char* msg, size_t index, size_t size) const;
+    static UpdateStatus do_init_from_parent(BPlusTreeBase* tree, ref_type ref, bool allow_create);
 };
 
 inline std::string_view collection_type_name(CollectionType col_type, bool uppercase = false)
@@ -492,7 +493,7 @@ class CollectionBaseImpl : public Interface, protected ArrayParent {
         if (m_parent) {
             try {
                 // Update the parent. Will throw if parent is not existing.
-                switch (m_parent->update_if_needed_with_status()) {
+                switch (m_parent->update_if_needed()) {
                     case UpdateStatus::Updated:
                         // Make sure to update next time around
                         m_content_version = 0;
@@ -524,7 +525,7 @@ class CollectionBaseImpl : public Interface, protected ArrayParent {
     {
         try {
             // `has_changed()` sneakily modifies internal state.
-            update_if_needed_with_status();
+            update_if_needed();
             if (m_last_content_version != m_content_version) {
                 m_last_content_version = m_content_version;
                 return true;
@@ -573,14 +574,12 @@ class CollectionBaseImpl : public Interface, protected ArrayParent {
     Obj m_obj_mem;
     std::shared_ptr<CollectionParent> m_col_parent;
     CollectionParent::Index m_index;
-    mutable size_t m_my_version = 0;
     ColKey m_col_key;
-    bool m_nullable = false;
-
     mutable uint_fast64_t m_content_version = 0;
-
     // Content version used by `has_changed()`.
     mutable uint_fast64_t m_last_content_version = 0;
+    mutable uint32_t m_parent_version = 0;
+    bool m_nullable = false;
 
     CollectionBaseImpl() = default;
     CollectionBaseImpl(const CollectionBaseImpl& other)
@@ -650,13 +649,14 @@ class CollectionBaseImpl : public Interface, protected ArrayParent {
 
     UpdateStatus get_update_status() const
     {
-        UpdateStatus status = m_parent ? m_parent->update_if_needed_with_status() : UpdateStatus::Detached;
+        UpdateStatus status = m_parent ? m_parent->update_if_needed() : UpdateStatus::Detached;
 
         if (status != UpdateStatus::Detached) {
             auto content_version = m_alloc->get_content_version();
-            if (content_version != m_content_version || m_my_version != m_parent->m_parent_version) {
+            auto parent_version = m_parent->parent_version();
+            if (content_version != m_content_version || m_parent_version != parent_version) {
                 m_content_version = content_version;
-                m_my_version = m_parent->m_parent_version;
+                m_parent_version = parent_version;
                 status = UpdateStatus::Updated;
             }
         }
@@ -667,18 +667,14 @@ class CollectionBaseImpl : public Interface, protected ArrayParent {
     /// Refresh the parent object (if needed) and compare version numbers.
     /// Return true if the collection should initialize from parent
     /// Throws if the owning object no longer exists.
-    bool should_update()
+    bool should_update() const
     {
         check_parent();
-        bool changed = m_parent->update_if_needed(); // Throws if the object does not exist.
-        auto content_version = m_alloc->get_content_version();
-
-        if (changed || content_version != m_content_version || m_my_version != m_parent->m_parent_version) {
-            m_content_version = content_version;
-            m_my_version = m_parent->m_parent_version;
-            return true;
+        auto status = get_update_status();
+        if (status == UpdateStatus::Detached) {
+            throw StaleAccessor("Parent no longer exists");
         }
-        return false;
+        return status == UpdateStatus::Updated;
     }
 
     void bump_content_version()
@@ -796,7 +792,7 @@ class CollectionBaseImpl : public Interface, protected ArrayParent {
     ///
     /// If no change has happened to the data, this function returns
     /// `UpdateStatus::NoChange`, and the caller is allowed to not do anything.
-    virtual UpdateStatus update_if_needed_with_status() const = 0;
+    virtual UpdateStatus update_if_needed() const = 0;
 };
 
 namespace _impl {
@@ -884,7 +880,7 @@ class ObjCollectionBase : public Interface, public _impl::ObjListProxy {
     /// `BPlusTree<T>`.
     virtual BPlusTree<ObjKey>* get_mutable_tree() const = 0;
 
-    /// Implements update_if_needed() in a way that ensures the consistency of
+    /// Implements `update_if_needed()` in a way that ensures the consistency of
     /// the unresolved list. Derived classes should call this instead of calling
     /// `update_if_needed()` on their inner accessor.
     UpdateStatus update_if_needed() const

src/realm/collection_parent.hpp

@@ -74,7 +74,7 @@ class CollectionParent : public std::enable_shared_from_this<CollectionParent> {
     using Index = StableIndex;
 
     // Return the nesting level of the parent
-    size_t get_level() const noexcept
+    uint8_t get_level() const noexcept
     {
         return m_level;
     }
@@ -106,21 +106,17 @@ class CollectionParent : public std::enable_shared_from_this<CollectionParent> {
 #else
     static constexpr size_t s_max_level = 100;
 #endif
-    size_t m_level = 0;
-    mutable size_t m_parent_version = 0;
+    uint8_t m_level = 0;
 
-    constexpr CollectionParent(size_t level = 0)
+    constexpr CollectionParent(uint8_t level = 0)
         : m_level(level)
     {
     }
 
     virtual ~CollectionParent();
     /// Update the accessor (and return `UpdateStatus::Detached` if the
     // collection is not initialized.
-    virtual UpdateStatus update_if_needed_with_status() const = 0;
-    /// Check if the storage version has changed and update if it has
-    /// Return true if the object was updated
-    virtual bool update_if_needed() const = 0;
+    virtual UpdateStatus update_if_needed() const = 0;
     /// Get owning object
     virtual const Obj& get_object() const noexcept = 0;
     /// Get the top ref from pareht
@@ -133,6 +129,8 @@ class CollectionParent : public std::enable_shared_from_this<CollectionParent> {
     /// Set the top ref in parent
     virtual void set_collection_ref(Index, ref_type ref, CollectionType) = 0;
 
+    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

src/realm/dictionary.cpp

@@ -651,10 +651,9 @@ size_t Dictionary::find_index(const Index& index) const
     return m_values->find_key(index.get_salt());
 }
 
-UpdateStatus Dictionary::update_if_needed_with_status() const
+UpdateStatus Dictionary::do_update_if_needed(bool allow_create) const
 {
-    auto status = Base::get_update_status();
-    switch (status) {
+    switch (get_update_status()) {
         case UpdateStatus::Detached: {
             m_dictionary_top.reset();
             return UpdateStatus::Detached;
@@ -667,27 +666,24 @@ UpdateStatus Dictionary::update_if_needed_with_status() const
             // perform lazy initialization by treating it as an update.
             [[fallthrough]];
         }
-        case UpdateStatus::Updated: {
-            // Try to initialize. If the dictionary is not initialized
-            // the function will return false;
-            bool attached = init_from_parent(false);
-            Base::update_content_version();
-            CollectionParent::m_parent_version++;
-            return attached ? UpdateStatus::Updated : UpdateStatus::Detached;
-        }
+        case UpdateStatus::Updated:
+            return init_from_parent(allow_create);
     }
     REALM_UNREACHABLE();
 }
 
+UpdateStatus Dictionary::update_if_needed() const
+{
+    constexpr bool allow_create = false;
+    return do_update_if_needed(allow_create);
+}
+
 void Dictionary::ensure_created()
 {
-    if (Base::should_update() || !(m_dictionary_top && m_dictionary_top->is_attached())) {
-        // When allow_create is true, init_from_parent will always succeed
-        // In case of errors, an exception is thrown.
-        constexpr bool allow_create = true;
-        init_from_parent(allow_create); // Throws
-        CollectionParent::m_parent_version++;
-        Base::update_content_version();
+    constexpr bool allow_create = true;
+    if (do_update_if_needed(allow_create) == UpdateStatus::Detached) {
+        // FIXME: untested
+        throw StaleAccessor("Dictionary no longer exists");
     }
 }
 
@@ -836,8 +832,9 @@ void Dictionary::clear()
     }
 }
 
-bool Dictionary::init_from_parent(bool allow_create) const
+UpdateStatus Dictionary::init_from_parent(bool allow_create) const
 {
+    Base::update_content_version();
     try {
         auto ref = Base::get_collection_ref();
         if ((ref || allow_create) && !m_dictionary_top) {
@@ -870,7 +867,7 @@ bool Dictionary::init_from_parent(bool allow_create) const
             // dictionary detached
             if (!allow_create) {
                 m_dictionary_top.reset();
-                return false;
+                return UpdateStatus::Detached;
             }
 
             // Create dictionary
@@ -880,7 +877,7 @@ bool Dictionary::init_from_parent(bool allow_create) const
             m_dictionary_top->update_parent();
         }
 
-        return true;
+        return UpdateStatus::Updated;
     }
     catch (...) {
         m_dictionary_top.reset();
@@ -1178,7 +1175,6 @@ ref_type Dictionary::get_collection_ref(Index index, CollectionType type) const
         throw realm::IllegalOperation(util::format("Not a %1", type));
     }
     throw StaleAccessor("This collection is no more");
-    return 0;
 }
 
 bool Dictionary::check_collection_ref(Index index, CollectionType type) const noexcept
@@ -1199,15 +1195,6 @@ void Dictionary::set_collection_ref(Index index, ref_type ref, CollectionType ty
     m_values->set(ndx, Mixed(ref, type));
 }
 
-bool Dictionary::update_if_needed() const
-{
-    auto status = update_if_needed_with_status();
-    if (status == UpdateStatus::Detached) {
-        throw StaleAccessor("CollectionList no longer exists");
-    }
-    return status == UpdateStatus::Updated;
-}
-
 /************************* DictionaryLinkValues *************************/
 
 DictionaryLinkValues::DictionaryLinkValues(const Obj& obj, ColKey col_key)

src/realm/dictionary.hpp

@@ -39,10 +39,7 @@ class Dictionary final : public CollectionBaseImpl<DictionaryBase>, public Colle
     using Base = CollectionBaseImpl<DictionaryBase>;
     class Iterator;
 
-    Dictionary()
-        : CollectionParent(0)
-    {
-    }
+    Dictionary() = default;
     ~Dictionary();
 
     Dictionary(const Obj& obj, ColKey col_key)
@@ -213,12 +210,15 @@ class Dictionary final : public CollectionBaseImpl<DictionaryBase>, public Colle
     {
         return get_obj().get_table();
     }
-    UpdateStatus update_if_needed_with_status() const override;
-    bool update_if_needed() const override;
+    UpdateStatus update_if_needed() const override;
     const Obj& get_object() const noexcept override
     {
         return get_obj();
     }
+    uint32_t parent_version() const noexcept override
+    {
+        return m_parent_version;
+    }
     ref_type get_collection_ref(Index, CollectionType) const override;
     bool check_collection_ref(Index, CollectionType) const noexcept override;
     void set_collection_ref(Index, ref_type ref, CollectionType) override;
@@ -241,7 +241,7 @@ class Dictionary final : public CollectionBaseImpl<DictionaryBase>, public Colle
 
     Dictionary(Allocator& alloc, ColKey col_key, ref_type ref);
 
-    bool init_from_parent(bool allow_create) const;
+    UpdateStatus init_from_parent(bool allow_create) const;
     Mixed do_get(size_t ndx) const;
     void do_erase(size_t ndx, Mixed key);
     Mixed do_get_key(size_t ndx) const;
@@ -263,12 +263,14 @@ class Dictionary final : public CollectionBaseImpl<DictionaryBase>, public Colle
     void do_accumulate(size_t* return_ndx, AggregateType& agg) const;
 
     void ensure_created();
-    inline bool update() const
+    bool update() const
     {
-        return update_if_needed_with_status() != UpdateStatus::Detached;
+        return update_if_needed() != UpdateStatus::Detached;
     }
     void verify() const;
     void get_key_type();
+
+    UpdateStatus do_update_if_needed(bool allow_create) const;
 };
 
 class Dictionary::Iterator {
@@ -461,7 +463,7 @@ class DictionaryLinkValues final : public ObjCollectionBase<CollectionBase> {
     // Overrides of ObjCollectionBase:
     UpdateStatus do_update_if_needed() const final
     {
-        return m_source.update_if_needed_with_status();
+        return m_source.update_if_needed();
     }
     BPlusTree<ObjKey>* get_mutable_tree() const final
     {