From 4de343cb99b9def3f54c0286f9ee1a3517cc7133 Mon Sep 17 00:00:00 2001 From: Thomas Goyne Date: Fri, 23 Feb 2024 13:48:45 -0800 Subject: [PATCH] Eliminate copies when accessing values from Bson types Returning things by value performs a deep copy, which is very expensive when those things are also bson containers. Re-align the naming with the convention names for the functions rather than being weird and different. --- CHANGELOG.md | 9 ++-- src/realm/object-store/sync/app.cpp | 30 +++++------ .../object-store/sync/app_credentials.cpp | 4 +- .../object-store/sync/mongo_collection.cpp | 52 +++++++++---------- .../object-store/sync/mongo_collection.hpp | 8 +-- src/realm/util/bson/bson.cpp | 4 +- src/realm/util/bson/bson.hpp | 46 ++++++++-------- src/realm/util/bson/indexed_map.hpp | 10 ++-- test/object-store/sync/app.cpp | 27 +++++----- test/object-store/sync/flx_sync.cpp | 8 +-- 10 files changed, 97 insertions(+), 101 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 95234c1806a..c1be30e2a1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) -* None. +* Fix a performance regression when reading values from Bson containers and revert some breaking changes to the Bson API ([PR #7377](https://github.com/realm/realm-core/pull/7377), since v14.0.0) ### Breaking changes * None. @@ -28,7 +28,6 @@ * You can now use query substitution for the @type argument ([#7289](https://github.com/realm/realm-core/issues/7289)) ### Fixed -* ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) * Fixed crash when adding a collection to an indexed Mixed property ([#7246](https://github.com/realm/realm-core/issues/7246), since 14.0.0-beta.0) * \[C-API] Fixed the return type of `realm_set_list` and `realm_set_dictionary` to be the newly inserted collection, similarly to the behavior of `list/dictionary_insert_collection` (PR [#7247](https://github.com/realm/realm-core/pull/7247), since 14.0.0-beta.0) * Throw an exception when trying to insert an embedded object into a list of Mixed ([#7254](https://github.com/realm/realm-core/issues/7254), since 14.0.0-beta.0) @@ -49,14 +48,14 @@ ----------- ### Internals -* to_json API changed according to https://docs.google.com/document/d/1YtJN0sC89LMb4UVcPKFIfwC0Hsi9Vj7sIEP2vHQzVcY/edit?usp=sharing. Links to not embedded objects will never be followed. +* to_json API changed according to https://docs.google.com/document/d/1YtJN0sC89LMb4UVcPKFIfwC0Hsi9Vj7sIEP2vHQzVcY/edit?usp=sharing. Links to non-embedded objects will never be followed. ---------------------------------------------- # 14.0.0-beta.0 Release notes ### Enhancements -* Storage of Decimal128 properties has been optimised so that the individual values will take up 0 bits (if all nulls), 32 bits, 64 bits or 128 bits depending on what is needed. (PR [#6111]https://github.com/realm/realm-core/pull/6111)) +* Storage of Decimal128 properties has been optimised so that the individual values will take up 0 bits (if all nulls), 32 bits, 64 bits or 128 bits depending on what is needed. (PR [#6111](https://github.com/realm/realm-core/pull/6111)) * You can have a collection embedded in any Mixed property (except Set). * Querying a specific entry in a collection (in particular 'first and 'last') is supported. (PR [#4269](https://github.com/realm/realm-core/issues/4269)) * Index on list of strings property now supported (PR [#7142](https://github.com/realm/realm-core/pull/7142)) @@ -67,7 +66,7 @@ * Fixed equality queries on a Mixed property with an index possibly returning the wrong result if values of different types happened to have the same StringIndex hash. ([6407](https://github.com/realm/realm-core/issues/6407) since v11.0.0-beta.5). * If you have more than 8388606 links pointing to one specific object, the program will crash. ([#6577](https://github.com/realm/realm-core/issues/6577), since v6.0.0) * Query for NULL value in Dictionary would give wrong results ([6748])(https://github.com/realm/realm-core/issues/6748), since v10.0.0) -* A Realm generated on a non-apple ARM 64 device and copied to another platform (and vice-versa) were non-portable due to a sorting order difference. This impacts strings or binaries that have their first difference at a non-ascii character. These items may not be found in a set, or in an indexed column if the strings had a long common prefix (> 200 characters). ([PR # 6670](https://github.com/realm/realm-core/pull/6670), since 2.0.0-rc7 for indexes, and since since the introduction of sets in v10.2.0) +* A Realm generated on a non-apple ARM 64 device and copied to another platform (and vice-versa) were non-portable due to a sorting order difference. This impacts strings or binaries that have their first difference at a non-ascii character. These items may not be found in a set, or in an indexed column if the strings had a long common prefix (> 200 characters). ([PR #6670](https://github.com/realm/realm-core/pull/6670), since 2.0.0-rc7 for indexes, and since since the introduction of sets in v10.2.0) ### Breaking changes * Support for upgrading from Realm files produced by RealmCore v5.23.9 or earlier is no longer supported. diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp index 79fa32cae33..a0f28084abc 100644 --- a/src/realm/object-store/sync/app.cpp +++ b/src/realm/object-store/sync/app.cpp @@ -694,20 +694,20 @@ void App::attach_auth_options(BsonDocument& body) log_debug("App: version info: platform: %1 version: %2 - sdk: %3 - sdk version: %4 - core version: %5", m_config.device_info.platform, m_config.device_info.platform_version, m_config.device_info.sdk, m_config.device_info.sdk_version, m_config.device_info.core_version); - options.append("appId", m_config.app_id); - options.append("platform", m_config.device_info.platform); - options.append("platformVersion", m_config.device_info.platform_version); - options.append("sdk", m_config.device_info.sdk); - options.append("sdkVersion", m_config.device_info.sdk_version); - options.append("cpuArch", m_config.device_info.cpu_arch); - options.append("deviceName", m_config.device_info.device_name); - options.append("deviceVersion", m_config.device_info.device_version); - options.append("frameworkName", m_config.device_info.framework_name); - options.append("frameworkVersion", m_config.device_info.framework_version); - options.append("coreVersion", m_config.device_info.core_version); - options.append("bundleId", m_config.device_info.bundle_id); - - body.append("options", BsonDocument({{"device", options}})); + options["appId"] = m_config.app_id; + options["platform"] = m_config.device_info.platform; + options["platformVersion"] = m_config.device_info.platform_version; + options["sdk"] = m_config.device_info.sdk; + options["sdkVersion"] = m_config.device_info.sdk_version; + options["cpuArch"] = m_config.device_info.cpu_arch; + options["deviceName"] = m_config.device_info.device_name; + options["deviceVersion"] = m_config.device_info.device_version; + options["frameworkName"] = m_config.device_info.framework_name; + options["frameworkVersion"] = m_config.device_info.framework_version; + options["coreVersion"] = m_config.device_info.core_version; + options["bundleId"] = m_config.device_info.bundle_id; + + body["options"] = BsonDocument({{"device", options}}); } void App::log_in_with_credentials( @@ -1339,7 +1339,7 @@ Request App::make_streaming_request(const std::shared_ptr& user, const {"name", name}, }; if (service_name) { - args.append("service", *service_name); + args["service"] = *service_name; } const auto args_json = Bson(args).to_string(); diff --git a/src/realm/object-store/sync/app_credentials.cpp b/src/realm/object-store/sync/app_credentials.cpp index cf67f173941..a438f38f55e 100644 --- a/src/realm/object-store/sync/app_credentials.cpp +++ b/src/realm/object-store/sync/app_credentials.cpp @@ -98,9 +98,9 @@ AppCredentials::AppCredentials(AuthProvider provider, : m_provider(provider) , m_payload(std::make_unique()) { - m_payload->append(kAppProviderKey, provider_type_from_enum(provider)); + (*m_payload)[kAppProviderKey] = provider_type_from_enum(provider); for (auto& [key, value] : values) { - m_payload->append(key, std::move(value)); + (*m_payload)[key] = std::move(value); } } diff --git a/src/realm/object-store/sync/mongo_collection.cpp b/src/realm/object-store/sync/mongo_collection.cpp index 4c0f8d45212..67ce0d08f21 100644 --- a/src/realm/object-store/sync/mongo_collection.cpp +++ b/src/realm/object-store/sync/mongo_collection.cpp @@ -279,15 +279,15 @@ void MongoCollection::call_function(const char* name, const bson::BsonDocument& static void set_options(BsonDocument& base_args, const MongoCollection::FindOptions& options) { if (options.limit) { - base_args.append("limit", *options.limit); + base_args["limit"] = *options.limit; } if (options.projection_bson) { - base_args.append("project", *options.projection_bson); + base_args["project"] = *options.projection_bson; } if (options.sort_bson) { - base_args.append("sort", *options.sort_bson); + base_args["sort"] = *options.sort_bson; } } @@ -295,7 +295,7 @@ void MongoCollection::find_bson(const BsonDocument& filter_bson, const FindOptio ResponseHandler>&& completion) try { auto base_args = m_base_operation_args; - base_args.append("query", filter_bson); + base_args["query"] = filter_bson; set_options(base_args, options); call_function("find", base_args, std::move(completion)); @@ -308,7 +308,7 @@ void MongoCollection::find_one_bson(const BsonDocument& filter_bson, const FindO ResponseHandler>&& completion) try { auto base_args = m_base_operation_args; - base_args.append("query", filter_bson); + base_args["query"] = filter_bson; set_options(base_args, options); call_function("findOne", base_args, std::move(completion)); } @@ -320,14 +320,14 @@ void MongoCollection::insert_one_bson(const BsonDocument& value_bson, ResponseHandler>&& completion) { auto base_args = m_base_operation_args; - base_args.append("document", value_bson); + base_args["document"] = value_bson; call_function("insertOne", base_args, std::move(completion)); } void MongoCollection::aggregate_bson(const BsonArray& pipline, ResponseHandler>&& completion) { auto base_args = m_base_operation_args; - base_args.append("pipeline", pipline); + base_args["pipeline"] = pipline; call_function("aggregate", base_args, std::move(completion)); } @@ -335,9 +335,9 @@ void MongoCollection::count_bson(const BsonDocument& filter_bson, int64_t limit, ResponseHandler>&& completion) { auto base_args = m_base_operation_args; - base_args.append("query", filter_bson); + base_args["query"] = filter_bson; if (limit != 0) { - base_args.append("limit", limit); + base_args["limit"] = limit; } call_function("count", base_args, std::move(completion)); } @@ -345,7 +345,7 @@ void MongoCollection::count_bson(const BsonDocument& filter_bson, int64_t limit, void MongoCollection::insert_many_bson(const BsonArray& documents, ResponseHandler>&& completion) { auto base_args = m_base_operation_args; - base_args.append("documents", documents); + base_args["documents"] = documents; call_function("insertMany", base_args, std::move(completion)); } @@ -353,7 +353,7 @@ void MongoCollection::delete_one_bson(const BsonDocument& filter_bson, ResponseHandler>&& completion) { auto base_args = m_base_operation_args; - base_args.append("query", filter_bson); + base_args["query"] = filter_bson; call_function("deleteOne", base_args, std::move(completion)); } @@ -361,7 +361,7 @@ void MongoCollection::delete_many_bson(const BsonDocument& filter_bson, ResponseHandler>&& completion) { auto base_args = m_base_operation_args; - base_args.append("query", filter_bson); + base_args["query"] = filter_bson; call_function("deleteMany", base_args, std::move(completion)); } @@ -369,9 +369,9 @@ void MongoCollection::update_one_bson(const BsonDocument& filter_bson, const Bso ResponseHandler>&& completion) { auto base_args = m_base_operation_args; - base_args.append("query", filter_bson); - base_args.append("update", update_bson); - base_args.append("upsert", upsert); + base_args["query"] = filter_bson; + base_args["update"] = update_bson; + base_args["upsert"] = upsert; call_function("updateOne", base_args, std::move(completion)); } @@ -379,9 +379,9 @@ void MongoCollection::update_many_bson(const BsonDocument& filter_bson, const Bs ResponseHandler>&& completion) { auto base_args = m_base_operation_args; - base_args.append("query", filter_bson); - base_args.append("update", update_bson); - base_args.append("upsert", upsert); + base_args["query"] = filter_bson; + base_args["update"] = update_bson; + base_args["upsert"] = upsert; call_function("updateMany", base_args, std::move(completion)); } @@ -390,8 +390,8 @@ void MongoCollection::find_one_and_update_bson(const BsonDocument& filter_bson, ResponseHandler>&& completion) { auto base_args = m_base_operation_args; - base_args.append("filter", filter_bson); - base_args.append("update", update_bson); + base_args["filter"] = filter_bson; + base_args["update"] = update_bson; options.set_bson(base_args); call_function("findOneAndUpdate", base_args, std::move(completion)); } @@ -401,8 +401,8 @@ void MongoCollection::find_one_and_replace_bson(const BsonDocument& filter_bson, ResponseHandler>&& completion) { auto base_args = m_base_operation_args; - base_args.append("filter", filter_bson); - base_args.append("update", replacement_bson); + base_args["filter"] = filter_bson; + base_args["update"] = replacement_bson; options.set_bson(base_args); call_function("findOneAndReplace", base_args, std::move(completion)); } @@ -412,7 +412,7 @@ void MongoCollection::find_one_and_delete_bson(const BsonDocument& filter_bson, ResponseHandler>&& completion) { auto base_args = m_base_operation_args; - base_args.append("filter", filter_bson); + base_args["filter"] = filter_bson; options.set_bson(base_args); call_function("findOneAndDelete", base_args, std::move(completion)); } @@ -568,8 +568,8 @@ void WatchStream::feed_sse(ServerSentEvent sse) if (parsed.type() != Bson::Type::Document) return; auto& obj = static_cast(parsed); - auto code = obj.at("error_code"); - auto msg = obj.at("error"); + auto& code = obj.at("error_code"); + auto& msg = obj.at("error"); if (code.type() != Bson::Type::String) return; if (msg.type() != Bson::Type::String) @@ -577,7 +577,7 @@ void WatchStream::feed_sse(ServerSentEvent sse) auto error_code = ErrorCodes::from_string(static_cast(code)); if (error_code == ErrorCodes::UnknownError) error_code = ErrorCodes::AppUnknownError; - m_error = std::make_unique(error_code, std::move(static_cast(msg))); + m_error = std::make_unique(error_code, std::move(static_cast(msg))); } catch (...) { return; // Use the default state. diff --git a/src/realm/object-store/sync/mongo_collection.hpp b/src/realm/object-store/sync/mongo_collection.hpp index fceef5d1da2..e9056f30b76 100644 --- a/src/realm/object-store/sync/mongo_collection.hpp +++ b/src/realm/object-store/sync/mongo_collection.hpp @@ -74,19 +74,19 @@ class MongoCollection { void set_bson(bson::BsonDocument& bson) const { if (upsert) { - bson.append("upsert", true); + bson["upsert"] = true; } if (return_new_document) { - bson.append("returnNewDocument", true); + bson["returnNewDocument"] = true; } if (projection_bson) { - bson.append("projection", *projection_bson); + bson["projection"] = *projection_bson; } if (sort_bson) { - bson.append("sort", *sort_bson); + bson["sort"] = *sort_bson; } } }; diff --git a/src/realm/util/bson/bson.cpp b/src/realm/util/bson/bson.cpp index 33960a8ffdb..fec28cf1562 100644 --- a/src/realm/util/bson/bson.cpp +++ b/src/realm/util/bson/bson.cpp @@ -622,7 +622,7 @@ Bson dom_elem_to_bson(const Json& json) case Json::value_t::array: { BsonArray out; for (auto&& elem : json) { - out.append(dom_elem_to_bson(elem)); + out.push_back(dom_elem_to_bson(elem)); } return Bson(std::move(out)); } @@ -786,7 +786,7 @@ Bson dom_obj_to_bson(const Json& json) BsonDocument out; for (auto&& [k, v] : json.items()) { - out.append(k, dom_elem_to_bson(v)); + out[k] = dom_elem_to_bson(v); } return out; } diff --git a/src/realm/util/bson/bson.hpp b/src/realm/util/bson/bson.hpp index f4e74eeb51a..99fb1df32cb 100644 --- a/src/realm/util/bson/bson.hpp +++ b/src/realm/util/bson/bson.hpp @@ -308,31 +308,40 @@ class BsonDocument : private IndexedMap { return size() == 0; } - void append(const std::string& key, const Bson& val) + Bson& operator[](const std::string& k) { - IndexedMap::operator[](key) = val; + return IndexedMap::operator[](k); } - Bson operator[](const std::string& k) const + Bson& at(const std::string& k) { - return at(k); + return IndexedMap::at(k); } - Bson at(const std::string& k) const + const Bson& at(const std::string& k) const { return IndexedMap::at(k); } - std::optional find(const std::string& key) const + Bson* find(const std::string& key) + { + auto& raw = entries(); + if (auto it = raw.find(key); it != raw.end()) { + return &it->second; + } + return nullptr; + } + + const Bson* find(const std::string& key) const { auto& raw = entries(); if (auto it = raw.find(key); it != raw.end()) { - return it->second; + return &it->second; } - return {}; + return nullptr; } - bool operator==(const BsonDocument& other) const + bool operator==(const BsonDocument& other) const noexcept { return static_cast*>(this)->entries() == other.entries(); } @@ -341,26 +350,13 @@ class BsonDocument : private IndexedMap { class BsonArray : private std::vector { public: using entry = Bson; + using vector::vector; using vector::begin; using vector::end; using vector::size; using vector::empty; - - BsonArray() {} - BsonArray(std::initializer_list entries) - : std::vector(entries) - { - } - - Bson operator[](size_t n) const - { - return std::vector::operator[](n); - } - - void append(const Bson& val) - { - std::vector::push_back(val); - } + using vector::push_back; + using vector::operator[]; bool operator==(const BsonArray& other) const { diff --git a/src/realm/util/bson/indexed_map.hpp b/src/realm/util/bson/indexed_map.hpp index dc796f36640..b8d211b9c58 100644 --- a/src/realm/util/bson/indexed_map.hpp +++ b/src/realm/util/bson/indexed_map.hpp @@ -110,6 +110,10 @@ class IndexedMap { { return m_map; } + std::unordered_map& entries() noexcept + { + return m_map; + } private: template @@ -243,11 +247,11 @@ typename IndexedMap::iterator IndexedMap::find(const std::string& k) const template T& IndexedMap::operator[](const std::string& k) { - auto entry = m_map.find(k); - if (entry == m_map.end()) { - m_keys.push_back(k); + if (auto entry = m_map.find(k); entry != m_map.end()) { + return entry->second; } + m_keys.push_back(k); return m_map[k]; } diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index 0e5e4b841f2..181ddb15858 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -1356,11 +1356,8 @@ TEST_CASE("app: call function", "[sync][app][function][baas]") { TestAppSession session; auto app = session.app(); - bson::BsonArray toSum; - for (int64_t i : {1, 2, 3, 4, 5}) { - toSum.append(i); - } - + bson::BsonArray toSum(5); + std::iota(toSum.begin(), toSum.end(), static_cast(1)); const auto checkFn = [](Optional&& sum, Optional&& error) { REQUIRE(!error); CHECK(*sum == 15); @@ -1493,7 +1490,7 @@ TEST_CASE("app: remote mongo client", "[sync][app][mongo][baas]") { CHECK(static_cast(*object_id) == cat_id_string); }); - person_document.append("dogs", bson::BsonArray({dog_object_id, dog2_object_id, dog3_object_id})); + person_document["dogs"] = bson::BsonArray({dog_object_id, dog2_object_id, dog3_object_id}); person_collection.insert_one(person_document, [&](Optional object_id, Optional error) { REQUIRE_FALSE(error); CHECK((*object_id).to_string() != ""); @@ -1574,7 +1571,7 @@ TEST_CASE("app: remote mongo client", "[sync][app][mongo][baas]") { dog2_object_id = static_cast(*object_id); }); - person_document.append("dogs", bson::BsonArray({dog_object_id, dog2_object_id})); + person_document["dogs"] = bson::BsonArray({dog_object_id, dog2_object_id}); person_collection.insert_one(person_document, [&](Optional object_id, Optional error) { REQUIRE_FALSE(error); CHECK((*object_id).to_string() != ""); @@ -1695,7 +1692,7 @@ TEST_CASE("app: remote mongo client", "[sync][app][mongo][baas]") { dog2_object_id = static_cast(*object_id); }); - person_document.append("dogs", bson::BsonArray({dog_object_id, dog2_object_id})); + person_document["dogs"] = bson::BsonArray({dog_object_id, dog2_object_id}); person_collection.insert_one(person_document, [&](Optional object_id, Optional error) { REQUIRE_FALSE(error); CHECK((*object_id).to_string() != ""); @@ -1874,9 +1871,9 @@ TEST_CASE("app: remote mongo client", "[sync][app][mongo][baas]") { REQUIRE(upserted_id == cat_id_string); }); - person_document.append("dogs", bson::BsonArray()); + person_document["dogs"] = bson::BsonArray(); bson::BsonDocument person_document_copy = bson::BsonDocument(person_document); - person_document_copy.append("dogs", bson::BsonArray({dog_object_id})); + person_document_copy["dogs"] = bson::BsonArray({dog_object_id}); person_collection.update_one(person_document, person_document, true, [&](MongoCollection::UpdateResult, Optional error) { REQUIRE_FALSE(error); @@ -1948,8 +1945,8 @@ TEST_CASE("app: remote mongo client", "[sync][app][mongo][baas]") { CHECK(static_cast(name) == "fido"); }); - person_document.append("dogs", bson::BsonArray({dog_object_id})); - person_document2.append("dogs", bson::BsonArray({dog_object_id})); + person_document["dogs"] = bson::BsonArray({dog_object_id}); + person_document2["dogs"] = bson::BsonArray({dog_object_id}); person_collection.insert_one(person_document, [&](Optional object_id, Optional error) { REQUIRE_FALSE(error); CHECK((*object_id).to_string() != ""); @@ -2003,9 +2000,9 @@ TEST_CASE("app: remote mongo client", "[sync][app][mongo][baas]") { bool processed = false; bson::BsonArray documents; - documents.append(dog_document); - documents.append(dog_document); - documents.append(dog_document); + documents.push_back(dog_document); + documents.push_back(dog_document); + documents.push_back(dog_document); dog_collection.insert_many(documents, [&](bson::BsonArray inserted_docs, Optional error) { REQUIRE_FALSE(error); diff --git a/test/object-store/sync/flx_sync.cpp b/test/object-store/sync/flx_sync.cpp index 6e048b78b9a..3ac457c8e3e 100644 --- a/test/object-store/sync/flx_sync.cpp +++ b/test/object-store/sync/flx_sync.cpp @@ -1980,10 +1980,10 @@ TEST_CASE("flx: geospatial", "[sync][flx][geospatial][baas]") { bson::BsonArray inner{}; REALM_ASSERT_3(polygon.points.size(), ==, 1); for (auto& point : polygon.points[0]) { - inner.append(bson::BsonArray{point.longitude, point.latitude}); + inner.push_back(bson::BsonArray{point.longitude, point.latitude}); } bson::BsonArray coords; - coords.append(inner); + coords.push_back(inner); bson::BsonDocument geo_bson{{{"type", "Polygon"}, {"coordinates", coords}}}; bson::BsonDocument filter{ {"location", bson::BsonDocument{{"$geoWithin", bson::BsonDocument{{"$geometry", geo_bson}}}}}}; @@ -1992,8 +1992,8 @@ TEST_CASE("flx: geospatial", "[sync][flx][geospatial][baas]") { auto make_circle_filter = [&](const GeoCircle& circle) -> bson::BsonDocument { bson::BsonArray coords{circle.center.longitude, circle.center.latitude}; bson::BsonArray inner; - inner.append(coords); - inner.append(circle.radius_radians); + inner.push_back(coords); + inner.push_back(circle.radius_radians); bson::BsonDocument filter{ {"location", bson::BsonDocument{{"$geoWithin", bson::BsonDocument{{"$centerSphere", inner}}}}}}; return filter;