Deprecate Object::obj() for Object::get_obj() (#6773)

* deprecate Object::obj for Object::get_obj * benchmark
realm · Jul 12, 2023 · bec9a4c · bec9a4c
1 parent ac91ecb
commit bec9a4c
Show file tree

Hide file tree

Showing 24 changed files with 261 additions and 185 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,7 +10,7 @@
 * Fix timestamp representation when serializing to json on different platforms. ([#5451](https://github.com/realm/realm-core/issues/5451)).
 
 ### Breaking changes
-* None.
+* Deprecate `Object::obj()` in favour of `Object::get_obj()` in order to provide better cache efficiency and keep `Obj` up to date when writes happened after then object instance is obtained.
 
 ### Compatibility
 * Fileformat: Generates files with format v23. Reads and automatically upgrade from fileformat v5.

diff --git a/src/realm/object-store/c_api/object.cpp b/src/realm/object-store/c_api/object.cpp
@@ -32,7 +32,7 @@ RLM_API bool realm_object_get_parent(const realm_object_t* object, realm_object_
                                      realm_class_key_t* class_key)
 {
     return wrap_err([&]() {
-        auto obj = object->obj().get_parent_object();
+        const auto& obj = object->get_obj().get_parent_object();
         if (class_key)
             *class_key = obj.get_table()->get_key().value;
 
@@ -145,7 +145,7 @@ RLM_API bool realm_object_delete(realm_object_t* obj)
 {
     return wrap_err([&]() {
         obj->verify_attached();
-        obj->obj().remove();
+        obj->get_obj().remove();
         return true;
     });
 }
@@ -209,7 +209,7 @@ RLM_API bool realm_object_add_int(realm_object_t* object, realm_property_key_t p
     REALM_ASSERT(object);
     return wrap_err([&]() {
         object->verify_attached();
-        object->obj().add_int(ColKey(property_key), value);
+        object->get_obj().add_int(ColKey(property_key), value);
         return true;
     });
 }
@@ -222,17 +222,17 @@ RLM_API bool realm_object_is_valid(const realm_object_t* obj)
 
 RLM_API realm_object_key_t realm_object_get_key(const realm_object_t* obj)
 {
-    return obj->obj().get_key().value;
+    return obj->get_obj().get_key().value;
 }
 
 RLM_API realm_class_key_t realm_object_get_table(const realm_object_t* obj)
 {
-    return obj->obj().get_table()->get_key().value;
+    return obj->get_obj().get_table()->get_key().value;
 }
 
 RLM_API realm_link_t realm_object_as_link(const realm_object_t* object)
 {
-    auto obj = object->obj();
+    const auto& obj = object->get_obj();
     auto table = obj.get_table();
     auto table_key = table->get_key();
     auto obj_key = obj.get_key();
@@ -264,7 +264,7 @@ RLM_API bool realm_get_values(const realm_object_t* obj, size_t num_values, cons
     return wrap_err([&]() {
         obj->verify_attached();
 
-        auto o = obj->obj();
+        auto o = obj->get_obj();
 
         for (size_t i = 0; i < num_values; ++i) {
             auto col_key = ColKey(properties[i]);
@@ -296,7 +296,7 @@ RLM_API bool realm_set_values(realm_object_t* obj, size_t num_values, const real
 {
     return wrap_err([&]() {
         obj->verify_attached();
-        auto o = obj->obj();
+        auto o = obj->get_obj();
         auto table = o.get_table();
 
         // Perform validation up front to avoid partial updates. This is
@@ -332,7 +332,7 @@ RLM_API realm_object_t* realm_set_embedded(realm_object_t* obj, realm_property_k
 {
     return wrap_err([&]() {
         obj->verify_attached();
-        auto o = obj->obj();
+        auto& o = obj->get_obj();
         return new realm_object_t({obj->get_realm(), o.create_and_set_linked_object(ColKey(col))});
     });
 }
@@ -341,7 +341,7 @@ RLM_API realm_object_t* realm_get_linked_object(realm_object_t* obj, realm_prope
 {
     return wrap_err([&]() {
         obj->verify_attached();
-        auto o = obj->obj().get_linked_object(ColKey(col));
+        const auto& o = obj->get_obj().get_linked_object(ColKey(col));
         return o ? new realm_object_t({obj->get_realm(), o}) : nullptr;
     });
 }
@@ -351,7 +351,7 @@ RLM_API realm_list_t* realm_get_list(realm_object_t* object, realm_property_key_
     return wrap_err([&]() {
         object->verify_attached();
 
-        auto obj = object->obj();
+        const auto& obj = object->get_obj();
         auto table = obj.get_table();
 
         auto col_key = ColKey(key);
@@ -370,7 +370,7 @@ RLM_API realm_set_t* realm_get_set(realm_object_t* object, realm_property_key_t
     return wrap_err([&]() {
         object->verify_attached();
 
-        auto obj = object->obj();
+        const auto& obj = object->get_obj();
         auto table = obj.get_table();
 
         auto col_key = ColKey(key);
@@ -389,7 +389,7 @@ RLM_API realm_dictionary_t* realm_get_dictionary(realm_object_t* object, realm_p
     return wrap_err([&]() {
         object->verify_attached();
 
-        auto obj = object->obj();
+        const auto& obj = object->get_obj();
         auto table = obj.get_table();
         auto col_key = ColKey(key);
         table->check_column(col_key);
@@ -407,7 +407,7 @@ RLM_API char* realm_object_to_string(realm_object_t* object)
     return wrap_err([&]() {
         object->verify_attached();
 
-        auto obj = object->obj();
+        const auto& obj = object->get_obj();
         return duplicate_string(obj.to_string());
     });
 }

diff --git a/src/realm/object-store/c_api/query.cpp b/src/realm/object-store/c_api/query.cpp
@@ -331,7 +331,7 @@ RLM_API realm_results_t* realm_get_backlinks(realm_object_t* object, realm_class
         object->verify_attached();
         auto realm = object->realm();
         auto source_table = realm->read_group().get_table(TableKey{source_table_key});
-        auto backlink_view = object->obj().get_backlink_view(source_table, ColKey{property_key});
+        auto backlink_view = object->get_obj().get_backlink_view(source_table, ColKey{property_key});
         return new realm_results_t{Results{realm, backlink_view}};
     });
 }
@@ -453,7 +453,7 @@ RLM_API bool realm_results_find_object(realm_results_t* results, realm_object_t*
 
     return wrap_err([&]() {
         if (out_index) {
-            *out_index = results->index_of(value->obj());
+            *out_index = results->index_of(value->get_obj());
             if (out_found && *out_index != realm::not_found)
                 *out_found = true;
         }

diff --git a/src/realm/object-store/c_api/schema.cpp b/src/realm/object-store/c_api/schema.cpp
@@ -184,7 +184,7 @@ RLM_API bool realm_get_value_by_property_index(const realm_object_t* object, siz
         auto& peristed_properties = object->get_object_schema().persisted_properties;
         REALM_ASSERT(prop_index < peristed_properties.size());
         auto col_key = peristed_properties[prop_index].column_key;
-        auto o = object->obj();
+        auto o = object->get_obj();
         auto val = o.get_any(col_key);
         auto converted = objkey_to_typed_link(val, col_key, *o.get_table());
         *out_value = to_capi(converted);

diff --git a/src/realm/object-store/c_api/types.hpp b/src/realm/object-store/c_api/types.hpp
@@ -273,8 +273,8 @@ struct realm_object : realm::c_api::WrapC, realm::Object {
     bool equals(const WrapC& other) const noexcept final
     {
         if (auto ptr = dynamic_cast<const realm_object_t*>(&other)) {
-            auto a = obj();
-            auto b = ptr->obj();
+            auto a = get_obj();
+            auto b = ptr->get_obj();
             return a.get_table() == b.get_table() && a.get_key() == b.get_key();
         }
         return false;

diff --git a/src/realm/object-store/collection.cpp b/src/realm/object-store/collection.cpp
@@ -34,7 +34,7 @@ Collection::Collection(PropertyType type) noexcept
 }
 
 Collection::Collection(const Object& parent_obj, const Property* prop)
-    : Collection(std::shared_ptr(parent_obj.get_realm()), parent_obj.obj().get_collection_ptr(prop->column_key),
+    : Collection(std::shared_ptr(parent_obj.get_realm()), parent_obj.get_obj().get_collection_ptr(prop->column_key),
                  prop->type)
 {
 }

diff --git a/src/realm/object-store/impl/object_accessor_impl.hpp b/src/realm/object-store/impl/object_accessor_impl.hpp
@@ -323,14 +323,14 @@ template <>
 inline Obj CppContext::unbox(std::any& v, CreatePolicy policy, ObjKey current_obj) const
 {
     if (auto object = std::any_cast<Object>(&v))
-        return object->obj();
+        return object->get_obj();
     if (auto obj = std::any_cast<Obj>(&v))
         return *obj;
     if (!policy.create)
         return Obj();
 
     REALM_ASSERT(object_schema);
-    return Object::create(const_cast<CppContext&>(*this), realm, *object_schema, v, policy, current_obj).obj();
+    return Object::create(const_cast<CppContext&>(*this), realm, *object_schema, v, policy, current_obj).get_obj();
 }
 
 template <>

diff --git a/src/realm/object-store/object.hpp b/src/realm/object-store/object.hpp
@@ -97,11 +97,18 @@ class Object {
     {
         return *m_object_schema;
     }
-    Obj obj() const
+    [[deprecated]] Obj obj() const
+    {
+        return m_obj;
+    }
+    const Obj& get_obj() const
+    {
+        return m_obj;
+    }
+    Obj& get_obj()
     {
         return m_obj;
     }
-
     bool is_valid() const
     {
         return m_obj.is_valid();

diff --git a/src/realm/object-store/thread_safe_reference.cpp b/src/realm/object-store/thread_safe_reference.cpp
@@ -124,7 +124,7 @@ class ThreadSafeReference::PayloadImpl<Object> : public ThreadSafeReference::Pay
 public:
     PayloadImpl(Object const& object)
         : Payload(*object.get_realm())
-        , m_key(object.obj().get_key())
+        , m_key(object.get_obj().get_key())
         , m_object_schema_name(object.get_object_schema().name)
     {
     }

diff --git a/test/object-store/benchmarks/object.cpp b/test/object-store/benchmarks/object.cpp
@@ -292,6 +292,71 @@ TEST_CASE("Benchmark object", "[benchmark]") {
         };
     }
 
+    SECTION("Update and Read multiple objects") {
+        auto table = r->read_group().get_table("class_all types");
+        ObjectSchema all_types = *r->schema().find("all types");
+        r->begin_transaction();
+        std::vector<Object> objs;
+        objs.reserve(1000);
+
+        std::random_device dev;
+        std::mt19937 rng(dev());
+        std::uniform_int_distribution<std::mt19937::result_type> dist(500, 5000);
+        int64_t benchmark_pk = dist(rng);
+        for (size_t i = 0; i < 1000; ++i) {
+            auto obj = Object::create(d, r, all_types,
+                                      std::any(AnyDict{
+                                          {"pk", benchmark_pk + i},
+                                          {"bool", true},
+                                          {"int", INT64_C(5)},
+                                          {"float", 2.2f},
+                                          {"double", 3.3},
+                                          {"string", "hello"s},
+                                          {"data", "olleh"s},
+                                          {"date", Timestamp(10, 20)},
+                                          {"object", AnyDict{{"value", INT64_C(10)}}},
+
+                                          {"bool array", AnyVec{true, false}},
+                                          {"int array", AnyVec{INT64_C(5), INT64_C(6)}},
+                                          {"float array", AnyVec{1.1f, 2.2f}},
+                                          {"double array", AnyVec{3.3, 4.4}},
+                                          {"string array", AnyVec{"a"s, "b"s, "c"s}},
+                                          {"data array", AnyVec{"d"s, "e"s, "f"s}},
+                                          {"date array", AnyVec{}},
+                                          {"object array", AnyVec{AnyDict{{"value", INT64_C(20)}}}},
+                                      }),
+                                      CreatePolicy::ForceCreate);
+            objs.push_back(obj);
+        }
+        r->commit_transaction();
+        advance_and_notify(*r);
+        ColKey col_int = table->get_column_key("int");
+        BENCHMARK_ADVANCED("update object get_obj()")(Catch::Benchmark::Chronometer meter)
+        {
+            r->begin_transaction();
+            meter.measure([&objs, &col_int] {
+                for (Object& obj : objs) {
+                    obj.get_obj().set(col_int, 10);
+                    REQUIRE(obj.get_obj().get<Int>(col_int) == 10);
+                }
+            });
+            r->commit_transaction();
+        };
+
+        // TODO remove this once Object::obj() is deleted.
+        //  BENCHMARK_ADVANCED("update object obj()")(Catch::Benchmark::Chronometer meter)
+        //  {
+        //      r->begin_transaction();
+        //      meter.measure([&objs, &col_int] {
+        //          for (Object& obj : objs) {
+        //              obj.obj().set(col_int, 10);
+        //              REQUIRE(obj.obj().get<Int>(col_int) == 10);
+        //          }
+        //      });
+        //      r->commit_transaction();
+        //  };
+    }
+
     SECTION("change notifications reporting") {
         auto table = r->read_group().get_table("class_person");
         Results result(r, table);

diff --git a/test/object-store/c_api/c_api.cpp b/test/object-store/c_api/c_api.cpp
@@ -1955,7 +1955,7 @@ TEST_CASE("C API", "[c_api]") {
 
         SECTION("native pointer mapping") {
             auto object = *static_cast<const realm::Object*>(_realm_object_get_native_ptr(obj1.get()));
-            auto obj = object.obj();
+            auto obj = object.get_obj();
             CHECK(obj.get<int64_t>(realm::ColKey(foo_int_key)) == int_val1.integer);
 
             auto obj1a = cptr_checked(_realm_object_from_native_copy(&object, sizeof(object)));
@@ -2451,7 +2451,7 @@ TEST_CASE("C API", "[c_api]") {
                 bool out_found;
                 CHECK(realm_query_find_first(q_decimal.get(), &out_value, &out_found));
                 CHECK(out_found);
-                auto link = obj1->obj().get_link();
+                auto link = obj1->get_obj().get_link();
                 realm_value_t expected;
                 expected.type = RLM_TYPE_LINK;
                 expected.link.target_table = link.get_table_key().value;
@@ -3867,8 +3867,8 @@ TEST_CASE("C API", "[c_api]") {
                     CHECK(!mixed_link.is_unresolved_link());
                     CHECK(mixed_link.is_type(type_TypedLink));
                     auto link = mixed_link.get_link();
-                    CHECK(link.get_obj_key() == obj1->obj().get_key());
-                    CHECK(link.get_table_key() == obj1->obj().get_table()->get_key());
+                    CHECK(link.get_obj_key() == obj1->get_obj().get_key());
+                    CHECK(link.get_table_key() == obj1->get_obj().get_table()->get_key());
                 });
 
                 SECTION("get") {

diff --git a/test/object-store/collection_fixtures.hpp b/test/object-store/collection_fixtures.hpp
@@ -645,7 +645,7 @@ struct SetOfObjects : public LinkedCollectionBase {
         ColKey col = get_link_col_key(obj.get_table());
         obj.get_linkset(col).clear();
     }
-    size_t count_unresolved_links(Obj)
+    size_t count_unresolved_links(Obj&)
     {
         return 0;
     }

diff --git a/test/object-store/dictionary.cpp b/test/object-store/dictionary.cpp
@@ -1350,7 +1350,7 @@ TEST_CASE("dictionary snapshot null", "[dictionary]") {
     StringData new_key("foo");
 
     auto target_obj = Object::create(ctx, r, *r->schema().find("target"), Any{AnyDict{{"id", Any{int64_t(42)}}}});
-    dict.insert(new_key, target_obj.obj().get_key());
+    dict.insert(new_key, target_obj.get_obj().get_key());
     r->commit_transaction();
     REQUIRE(values.size() == 1);
     REQUIRE(snapshot.size() == 0);
@@ -1362,13 +1362,13 @@ TEST_CASE("dictionary snapshot null", "[dictionary]") {
     r->commit_transaction();
     REQUIRE(values.size() == 0);
     REQUIRE(snapshot.size() == 1);
-    auto obj_link = ObjLink{target_obj.obj().get_table()->get_key(), target_obj.obj().get_key()};
+    auto obj_link = ObjLink{target_obj.get_obj().get_table()->get_key(), target_obj.get_obj().get_key()};
     REQUIRE(snapshot.get_any(0) == Mixed{obj_link});
 
     // a snapshot retains an entry for a link when the underlying object is deleted
     // but the snapshot link is nullified
     r->begin_transaction();
-    target_obj.obj().remove();
+    target_obj.get_obj().remove();
     r->commit_transaction();
     REQUIRE(values.size() == 0);
     REQUIRE(snapshot.size() == 1);

diff --git a/test/object-store/frozen_objects.cpp b/test/object-store/frozen_objects.cpp
@@ -544,8 +544,8 @@ TEST_CASE("Reclaim Frozen", "[reclaim_frozen]") {
         auto table2 = realm2->read_group().get_table(table_key);
         auto& o = entry.o;
         o = Object(realm2, table2->get_object(key));
-        entry.value = o.obj().get<Int>(col);
-        entry.link = o.obj().get<ObjKey>(link_col);
+        entry.value = o.get_obj().get<Int>(col);
+        entry.link = o.get_obj().get<ObjKey>(link_col);
         auto linked = table2->get_object(entry.link);
         entry.linked_value = linked.get<Int>(col);
         // add a dummy notification callback to later exercise the notification machinery
@@ -568,8 +568,8 @@ TEST_CASE("Reclaim Frozen", "[reclaim_frozen]") {
         for (int k = 0; k < num_checks_pr_trans; ++k) {
             auto& entry = refs[(unsigned)random_int() % num_pending_transactions];
             if (entry.realm) {
-                CHECK(entry.value == entry.o.obj().get<Int>(col));
-                auto link = entry.o.obj().get<ObjKey>(link_col);
+                CHECK(entry.value == entry.o.get_obj().get<Int>(col));
+                auto link = entry.o.get_obj().get<ObjKey>(link_col);
                 CHECK(link == entry.link);
                 auto table = entry.realm->read_group().get_table(table_key);
                 auto linked_value = table->get_object(link).get<Int>(col);