From da28a29c7ff95820346eb4228dc5a0c763efcfa6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Tue, 31 Jan 2023 17:09:41 +0100 Subject: [PATCH 01/56] Pass properties when creating vertices --- src/query/v2/plan/operator.cpp | 56 +++++++++++-------- src/query/v2/request_router.hpp | 4 +- tests/unit/mock_helpers.hpp | 2 +- .../unit/query_v2_create_node_multiframe.cpp | 2 +- tests/unit/query_v2_expression_evaluator.cpp | 2 +- 5 files changed, 38 insertions(+), 28 deletions(-) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index aa9743878..2c2f07116 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -227,22 +227,27 @@ class DistributedCreateNodeCursor : public Cursor { ExpressionEvaluator evaluator(&frame, context.symbol_table, context.evaluation_context, nullptr, storage::v3::View::NEW); if (const auto *node_info_properties = std::get_if(&node_info_.properties)) { - for (const auto &[key, value_expression] : *node_info_properties) { + for (const auto &[property, value_expression] : *node_info_properties) { TypedValue val = value_expression->Accept(evaluator); - if (context.request_router->IsPrimaryKey(primary_label, key)) { - rqst.primary_key.push_back(TypedValueToValue(val)); - pk.push_back(TypedValueToValue(val)); + auto msgs_value = TypedValueToValue(val); + if (context.request_router->IsPrimaryProperty(primary_label, property)) { + rqst.primary_key.push_back(msgs_value); + pk.push_back(std::move(msgs_value)); + } else { + rqst.properties.emplace_back(property, std::move(msgs_value)); } } } else { auto property_map = evaluator.Visit(*std::get(node_info_.properties)).ValueMap(); - for (const auto &[key, value] : property_map) { - auto key_str = std::string(key); - auto property_id = context.request_router->NameToProperty(key_str); - if (context.request_router->IsPrimaryKey(primary_label, property_id)) { - rqst.primary_key.push_back(TypedValueToValue(value)); - pk.push_back(TypedValueToValue(value)); - } + for (const auto &[property, typed_value] : property_map) { + auto property_str = std::string(property); + auto property_id = context.request_router->NameToProperty(property_str); + auto msgs_value = TypedValueToValue(typed_value); + if (context.request_router->IsPrimaryProperty(primary_label, property_id)) { + rqst.primary_key.push_back(msgs_value); + pk.push_back(std::move(msgs_value)); + } else + rqst.properties.emplace_back(property_id, std::move(msgs_value)); } } @@ -280,22 +285,27 @@ class DistributedCreateNodeCursor : public Cursor { ExpressionEvaluator evaluator(&frame, context.symbol_table, context.evaluation_context, nullptr, storage::v3::View::NEW); if (const auto *node_info_properties = std::get_if(&node_info_.properties)) { - for (const auto &[key, value_expression] : *node_info_properties) { + for (const auto &[property, value_expression] : *node_info_properties) { TypedValue val = value_expression->Accept(evaluator); - if (context.request_router->IsPrimaryKey(primary_label, key)) { - rqst.primary_key.push_back(TypedValueToValue(val)); - pk.push_back(TypedValueToValue(val)); + auto msgs_value = TypedValueToValue(val); + if (context.request_router->IsPrimaryProperty(primary_label, property)) { + rqst.primary_key.push_back(msgs_value); + pk.push_back(std::move(msgs_value)); + } else { + rqst.properties.emplace_back(property, std::move(msgs_value)); } } } else { auto property_map = evaluator.Visit(*std::get(node_info_.properties)).ValueMap(); - for (const auto &[key, value] : property_map) { - auto key_str = std::string(key); - auto property_id = context.request_router->NameToProperty(key_str); - if (context.request_router->IsPrimaryKey(primary_label, property_id)) { - rqst.primary_key.push_back(TypedValueToValue(value)); - pk.push_back(TypedValueToValue(value)); - } + for (const auto &[property, typed_value] : property_map) { + auto property_str = std::string(property); + auto property_id = context.request_router->NameToProperty(property_str); + auto msgs_value = TypedValueToValue(typed_value); + if (context.request_router->IsPrimaryProperty(primary_label, property_id)) { + rqst.primary_key.push_back(msgs_value); + pk.push_back(std::move(msgs_value)); + } else + rqst.properties.emplace_back(property_id, std::move(msgs_value)); } } @@ -2820,7 +2830,7 @@ class DistributedCreateExpandCursor : public Cursor { const auto set_vertex = [&context](const auto &vertex, auto &vertex_id) { vertex_id.first = vertex.PrimaryLabel(); for (const auto &[key, val] : vertex.Properties()) { - if (context.request_router->IsPrimaryKey(vertex_id.first.id, key)) { + if (context.request_router->IsPrimaryProperty(vertex_id.first.id, key)) { vertex_id.second.push_back(val); } } diff --git a/src/query/v2/request_router.hpp b/src/query/v2/request_router.hpp index bf8c93566..a8326d900 100644 --- a/src/query/v2/request_router.hpp +++ b/src/query/v2/request_router.hpp @@ -117,7 +117,7 @@ class RequestRouterInterface { virtual std::optional MaybeNameToEdgeType(const std::string &name) const = 0; virtual std::optional MaybeNameToLabel(const std::string &name) const = 0; virtual bool IsPrimaryLabel(storage::v3::LabelId label) const = 0; - virtual bool IsPrimaryKey(storage::v3::LabelId primary_label, storage::v3::PropertyId property) const = 0; + virtual bool IsPrimaryProperty(storage::v3::LabelId primary_label, storage::v3::PropertyId property) const = 0; virtual std::optional> AllocateInitialEdgeIds(io::Address coordinator_address) = 0; virtual void InstallSimulatorTicker(std::function tick_simulator) = 0; @@ -231,7 +231,7 @@ class RequestRouter : public RequestRouterInterface { return edge_types_.IdToName(id.AsUint()); } - bool IsPrimaryKey(storage::v3::LabelId primary_label, storage::v3::PropertyId property) const override { + bool IsPrimaryProperty(storage::v3::LabelId primary_label, storage::v3::PropertyId property) const override { const auto schema_it = shards_map_.schemas.find(primary_label); MG_ASSERT(schema_it != shards_map_.schemas.end(), "Invalid primary label id: {}", primary_label.AsUint()); diff --git a/tests/unit/mock_helpers.hpp b/tests/unit/mock_helpers.hpp index 15f264cac..5ce73538a 100644 --- a/tests/unit/mock_helpers.hpp +++ b/tests/unit/mock_helpers.hpp @@ -41,7 +41,7 @@ class MockedRequestRouter : public RequestRouterInterface { MOCK_METHOD(std::optional, MaybeNameToEdgeType, (const std::string &), (const)); MOCK_METHOD(std::optional, MaybeNameToLabel, (const std::string &), (const)); MOCK_METHOD(bool, IsPrimaryLabel, (storage::v3::LabelId), (const)); - MOCK_METHOD(bool, IsPrimaryKey, (storage::v3::LabelId, storage::v3::PropertyId), (const)); + MOCK_METHOD(bool, IsPrimaryProperty, (storage::v3::LabelId, storage::v3::PropertyId), (const)); MOCK_METHOD((std::optional>), AllocateInitialEdgeIds, (io::Address)); MOCK_METHOD(void, InstallSimulatorTicker, (std::function)); MOCK_METHOD(const std::vector &, GetSchemaForLabel, (storage::v3::LabelId), (const)); diff --git a/tests/unit/query_v2_create_node_multiframe.cpp b/tests/unit/query_v2_create_node_multiframe.cpp index b298d2781..a2d5b161d 100644 --- a/tests/unit/query_v2_create_node_multiframe.cpp +++ b/tests/unit/query_v2_create_node_multiframe.cpp @@ -58,7 +58,7 @@ TEST(CreateNodeTest, CreateNodeCursor) { MockedRequestRouter router; EXPECT_CALL(router, CreateVertices(_)).Times(1).WillOnce(Return(std::vector{})); EXPECT_CALL(router, IsPrimaryLabel(_)).WillRepeatedly(Return(true)); - EXPECT_CALL(router, IsPrimaryKey(_, _)).WillRepeatedly(Return(true)); + EXPECT_CALL(router, IsPrimaryProperty(_, _)).WillRepeatedly(Return(true)); auto context = MakeContext(ast, symbol_table, &router, &id_alloc); auto multi_frame = CreateMultiFrame(context.symbol_table.max_position()); cursor->PullMultiple(multi_frame, context); diff --git a/tests/unit/query_v2_expression_evaluator.cpp b/tests/unit/query_v2_expression_evaluator.cpp index 0000e62b2..6b1c23816 100644 --- a/tests/unit/query_v2_expression_evaluator.cpp +++ b/tests/unit/query_v2_expression_evaluator.cpp @@ -123,7 +123,7 @@ class MockedRequestRouter : public RequestRouterInterface { bool IsPrimaryLabel(LabelId label) const override { return true; } - bool IsPrimaryKey(LabelId primary_label, PropertyId property) const override { return true; } + bool IsPrimaryProperty(LabelId primary_label, PropertyId property) const override { return true; } std::optional> AllocateInitialEdgeIds(io::Address coordinator_address) override { return {}; From a38401130e44951a5a4263a88223630d2238e4c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 1 Feb 2023 13:24:58 +0100 Subject: [PATCH 02/56] Set vertex id in `Expand` properly --- src/query/v2/plan/operator.cpp | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index a4b893c5c..e885ed029 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -2949,27 +2949,16 @@ class DistributedCreateExpandCursor : public Cursor { const auto &v1 = v1_value.ValueVertex(); const auto &v2 = OtherVertex(frame); - // Set src and dest vertices - // TODO(jbajic) Currently we are only handling scenario where vertices - // are matched - const auto set_vertex = [&context](const auto &vertex, auto &vertex_id) { - vertex_id.first = vertex.PrimaryLabel(); - for (const auto &[key, val] : vertex.Properties()) { - if (context.request_router->IsPrimaryProperty(vertex_id.first.id, key)) { - vertex_id.second.push_back(val); - } - } - }; std::invoke([&]() { switch (edge_info.direction) { case EdgeAtom::Direction::IN: { - set_vertex(v2, request.src_vertex); - set_vertex(v1, request.dest_vertex); + request.src_vertex = v2.Id(); + request.dest_vertex = v1.Id(); break; } case EdgeAtom::Direction::OUT: { - set_vertex(v1, request.src_vertex); - set_vertex(v2, request.dest_vertex); + request.src_vertex = v1.Id(); + request.dest_vertex = v2.Id(); break; } case EdgeAtom::Direction::BOTH: From b136cd71d2f078f85d221fe54b3f102b6157f789 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 1 Feb 2023 14:22:47 +0100 Subject: [PATCH 03/56] Fix `DistributedCreatedNodeCursor` in case of `UNWIND` --- src/query/v2/plan/operator.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index e885ed029..2e16b3d9d 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -218,6 +218,7 @@ class DistributedCreateNodeCursor : public Cursor { } std::vector NodeCreationInfoToRequest(ExecutionContext &context, Frame &frame) { + primary_keys_.clear(); std::vector requests; msgs::PrimaryKey pk; msgs::NewVertex rqst; @@ -273,6 +274,7 @@ class DistributedCreateNodeCursor : public Cursor { } std::vector NodeCreationInfoToRequests(ExecutionContext &context, MultiFrame &multi_frame) { + primary_keys_.clear(); std::vector requests; auto multi_frame_modifier = multi_frame.GetValidFramesModifier(); for (auto &frame : multi_frame_modifier) { From 7be66f0c540ed0d926e0751736e2d6ff90e8d916 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 1 Feb 2023 14:24:04 +0100 Subject: [PATCH 04/56] Add unwind based dataset creator --- tests/mgbench/dataset_creator_unwind.py | 139 ++++++++++++++++++ .../accesscontrol_large.shard_configuration | 6 +- .../accesscontrol_medium.shard_configuration | 6 +- .../accesscontrol_small.shard_configuration | 6 +- 4 files changed, 154 insertions(+), 3 deletions(-) create mode 100644 tests/mgbench/dataset_creator_unwind.py diff --git a/tests/mgbench/dataset_creator_unwind.py b/tests/mgbench/dataset_creator_unwind.py new file mode 100644 index 000000000..c9f9a12df --- /dev/null +++ b/tests/mgbench/dataset_creator_unwind.py @@ -0,0 +1,139 @@ +# Copyright 2022 Memgraph Ltd. +# +# Use of this software is governed by the Business Source License +# included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source +# License, and you may not use this file except in compliance with the Business Source License. +# +# As of the Change Date specified in that file, in accordance with +# the Business Source License, use of this software will be governed +# by the Apache License, Version 2.0, included in the file +# licenses/APL.txt. + +import argparse +import random + +import helpers + +# Explaination of datasets: +# - empty_only_index: contains index; contains no data +# - small: contains index; contains data (small dataset) +# +# Datamodel is as follow: +# +# ┌──────────────┐ +# │ Permission │ +# ┌────────────────┐ │ Schema:uuid │ ┌────────────┐ +# │:IS_FOR_IDENTITY├────┤ Index:name ├───┤:IS_FOR_FILE│ +# └┬───────────────┘ └──────────────┘ └────────────┤ +# │ │ +# ┌──────▼──────────────┐ ┌──▼────────────────┐ +# │ Identity │ │ File │ +# │ Schema:uuid │ │ Schema:uuid │ +# │ Index:email │ │ Index:name │ +# └─────────────────────┘ │ Index:platformId │ +# └───────────────────┘ +# +# - File: attributes: ["uuid", "name", "platformId"] +# - Permission: attributes: ["uuid", "name"] +# - Identity: attributes: ["uuid", "email"] +# +# Indexes: +# - File: [File(uuid), File(platformId), File(name)] +# - Permission: [Permission(uuid), Permission(name)] +# - Identity: [Identity(uuid), Identity(email)] +# +# Edges: +# - (:Permission)-[:IS_FOR_FILE]->(:File) +# - (:Permission)-[:IS_FOR_IDENTITYR]->(:Identity) +# +# AccessControl specific: uuid is the schema + + +def main(): + parser = argparse.ArgumentParser() + parser.add_argument("--number_of_identities", type=int, default=10) + parser.add_argument("--number_of_files", type=int, default=10) + parser.add_argument("--percentage_of_permissions", type=float, default=1.0) + parser.add_argument("--filename", default="dataset.cypher") + + args = parser.parse_args() + + number_of_identities = args.number_of_identities + number_of_files = args.number_of_files + percentage_of_permissions = args.percentage_of_permissions + filename = args.filename + + assert number_of_identities >= 0 + assert number_of_files >= 0 + assert percentage_of_permissions > 0.0 and percentage_of_permissions <= 1.0 + assert filename != "" + + with open(filename, "w") as f: + f.write("MATCH (n) DETACH DELETE n;\n") + + # Create the indexes + f.write("CREATE INDEX ON :File;\n") + f.write("CREATE INDEX ON :Permission;\n") + f.write("CREATE INDEX ON :Identity;\n") + f.write("CREATE INDEX ON :File(platformId);\n") + f.write("CREATE INDEX ON :File(name);\n") + f.write("CREATE INDEX ON :Permission(name);\n") + f.write("CREATE INDEX ON :Identity(email);\n") + + # Create extra index: in distributed, this will be the schema + f.write("CREATE INDEX ON :File(uuid);\n") + f.write("CREATE INDEX ON :Permission(uuid);\n") + f.write("CREATE INDEX ON :Identity(uuid);\n") + + uuid = 1 + + # Create the nodes File + f.write("UNWIND [") + for index in range(0, number_of_files): + if index != 0: + f.write(",") + f.write(f'\n {{uuid: {uuid}, platformId: "platform_id", name: "name_file_{uuid}"}}') + uuid += 1 + f.write("\n] AS props CREATE (:File {uuid: props.uuid, platformId: props.platformId, name: props.name});\n") + + identities = [] + f.write("UNWIND [") + # Create the nodes Identity + for index in range(0, number_of_identities): + if index != 0: + f.write(",") + f.write(f'\n {{uuid: {uuid}, name: "mail_{uuid}@something.com"}}') + uuid += 1 + f.write("\n] AS props CREATE (:Identity {uuid: props.uuid, name: props.name});\n") + + f.write("UNWIND [") + wrote_anything = False + for outer_index in range(0, number_of_files): + for inner_index in range(0, number_of_identities): + + file_uuid = outer_index + 1 + identity_uuid = number_of_files + inner_index + 1 + + if random.random() <= percentage_of_permissions: + + if wrote_anything: + f.write(",") + + f.write( + f'\n {{permUuid: {uuid}, permName: "name_permission_{uuid}", fileUuid: {file_uuid}, identityUuid: {identity_uuid}}}' + ) + wrote_anything = True + uuid += 1 + f.write( + """ +\n] AS props +MATCH (file:File {uuid:props.fileUuid}), (identity:Identity {uuid: props.identityUuid}) +CREATE (permission:Permission {uuid: props.permUuid, name: props.permName}) +CREATE (permission)-[: IS_FOR_FILE]->(file) +CREATE (permission)-[: IS_FOR_IDENTITY]->(identity); +""" + ) + + +if __name__ == "__main__": + main() diff --git a/tests/mgbench/splitfiles/accesscontrol_large.shard_configuration b/tests/mgbench/splitfiles/accesscontrol_large.shard_configuration index 34dca66be..d2138ec93 100644 --- a/tests/mgbench/splitfiles/accesscontrol_large.shard_configuration +++ b/tests/mgbench/splitfiles/accesscontrol_large.shard_configuration @@ -1,8 +1,12 @@ -4 +8 uuid email name platformId +permUuid +permName +fileUuid +identityUuid 2 IS_FOR_IDENTITY IS_FOR_FILE diff --git a/tests/mgbench/splitfiles/accesscontrol_medium.shard_configuration b/tests/mgbench/splitfiles/accesscontrol_medium.shard_configuration index a807e783f..f05ee8993 100644 --- a/tests/mgbench/splitfiles/accesscontrol_medium.shard_configuration +++ b/tests/mgbench/splitfiles/accesscontrol_medium.shard_configuration @@ -1,8 +1,12 @@ -4 +8 uuid email name platformId +permUuid +permName +fileUuid +identityUuid 2 IS_FOR_IDENTITY IS_FOR_FILE diff --git a/tests/mgbench/splitfiles/accesscontrol_small.shard_configuration b/tests/mgbench/splitfiles/accesscontrol_small.shard_configuration index 9c11b6258..2cce1ccef 100644 --- a/tests/mgbench/splitfiles/accesscontrol_small.shard_configuration +++ b/tests/mgbench/splitfiles/accesscontrol_small.shard_configuration @@ -1,8 +1,12 @@ -4 +8 uuid email name platformId +permUuid +permName +fileUuid +identityUuid 2 IS_FOR_IDENTITY IS_FOR_FILE From 24ae6069f0dabe9cfb468c4ff857f2ec34041d0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 1 Feb 2023 14:31:56 +0100 Subject: [PATCH 05/56] Split edge creation into batches --- tests/mgbench/dataset_creator_unwind.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/mgbench/dataset_creator_unwind.py b/tests/mgbench/dataset_creator_unwind.py index c9f9a12df..4fe537275 100644 --- a/tests/mgbench/dataset_creator_unwind.py +++ b/tests/mgbench/dataset_creator_unwind.py @@ -107,7 +107,7 @@ def main(): f.write("\n] AS props CREATE (:Identity {uuid: props.uuid, name: props.name});\n") f.write("UNWIND [") - wrote_anything = False + created = 0 for outer_index in range(0, number_of_files): for inner_index in range(0, number_of_identities): @@ -116,17 +116,27 @@ def main(): if random.random() <= percentage_of_permissions: - if wrote_anything: + if created > 0: f.write(",") f.write( f'\n {{permUuid: {uuid}, permName: "name_permission_{uuid}", fileUuid: {file_uuid}, identityUuid: {identity_uuid}}}' ) - wrote_anything = True + created += 1 uuid += 1 + + if created == 5000: + f.write( + """\n] AS props +MATCH (file:File {uuid:props.fileUuid}), (identity:Identity {uuid: props.identityUuid}) +CREATE (permission:Permission {uuid: props.permUuid, name: props.permName}) +CREATE (permission)-[: IS_FOR_FILE]->(file) +CREATE (permission)-[: IS_FOR_IDENTITY]->(identity); +UNWIND [""" + ) + created = 0 f.write( - """ -\n] AS props + """\n] AS props MATCH (file:File {uuid:props.fileUuid}), (identity:Identity {uuid: props.identityUuid}) CREATE (permission:Permission {uuid: props.permUuid, name: props.permName}) CREATE (permission)-[: IS_FOR_FILE]->(file) From 50327254e0e5f88e97b1bad83d2a28295ae30003 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 1 Feb 2023 14:59:50 +0100 Subject: [PATCH 06/56] Make queries into a single line --- tests/mgbench/dataset_creator_unwind.py | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/tests/mgbench/dataset_creator_unwind.py b/tests/mgbench/dataset_creator_unwind.py index 4fe537275..84434ba6f 100644 --- a/tests/mgbench/dataset_creator_unwind.py +++ b/tests/mgbench/dataset_creator_unwind.py @@ -92,9 +92,9 @@ def main(): for index in range(0, number_of_files): if index != 0: f.write(",") - f.write(f'\n {{uuid: {uuid}, platformId: "platform_id", name: "name_file_{uuid}"}}') + f.write(f' {{uuid: {uuid}, platformId: "platform_id", name: "name_file_{uuid}"}}') uuid += 1 - f.write("\n] AS props CREATE (:File {uuid: props.uuid, platformId: props.platformId, name: props.name});\n") + f.write("] AS props CREATE (:File {uuid: props.uuid, platformId: props.platformId, name: props.name});\n") identities = [] f.write("UNWIND [") @@ -102,9 +102,9 @@ def main(): for index in range(0, number_of_identities): if index != 0: f.write(",") - f.write(f'\n {{uuid: {uuid}, name: "mail_{uuid}@something.com"}}') + f.write(f' {{uuid: {uuid}, name: "mail_{uuid}@something.com"}}') uuid += 1 - f.write("\n] AS props CREATE (:Identity {uuid: props.uuid, name: props.name});\n") + f.write("] AS props CREATE (:Identity {uuid: props.uuid, name: props.name});\n") f.write("UNWIND [") created = 0 @@ -120,28 +120,18 @@ def main(): f.write(",") f.write( - f'\n {{permUuid: {uuid}, permName: "name_permission_{uuid}", fileUuid: {file_uuid}, identityUuid: {identity_uuid}}}' + f' {{permUuid: {uuid}, permName: "name_permission_{uuid}", fileUuid: {file_uuid}, identityUuid: {identity_uuid}}}' ) created += 1 uuid += 1 if created == 5000: f.write( - """\n] AS props -MATCH (file:File {uuid:props.fileUuid}), (identity:Identity {uuid: props.identityUuid}) -CREATE (permission:Permission {uuid: props.permUuid, name: props.permName}) -CREATE (permission)-[: IS_FOR_FILE]->(file) -CREATE (permission)-[: IS_FOR_IDENTITY]->(identity); -UNWIND [""" + "] AS props MATCH (file:File {uuid:props.fileUuid}), (identity:Identity {uuid: props.identityUuid}) CREATE (permission:Permission {uuid: props.permUuid, name: props.permName}) CREATE (permission)-[: IS_FOR_FILE]->(file) CREATE (permission)-[: IS_FOR_IDENTITY]->(identity);\nUNWIND [" ) created = 0 f.write( - """\n] AS props -MATCH (file:File {uuid:props.fileUuid}), (identity:Identity {uuid: props.identityUuid}) -CREATE (permission:Permission {uuid: props.permUuid, name: props.permName}) -CREATE (permission)-[: IS_FOR_FILE]->(file) -CREATE (permission)-[: IS_FOR_IDENTITY]->(identity); -""" + "] AS props MATCH (file:File {uuid:props.fileUuid}), (identity:Identity {uuid: props.identityUuid}) CREATE (permission:Permission {uuid: props.permUuid, name: props.permName}) CREATE (permission)-[: IS_FOR_FILE]->(file) CREATE (permission)-[: IS_FOR_IDENTITY]->(identity);" ) From c9a0c15c16ebb786625566019fc5c2a8c60c47aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 1 Feb 2023 16:17:06 +0100 Subject: [PATCH 07/56] Make frames invalid on consumption --- src/query/v2/plan/operator.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index 2e16b3d9d..e24e4d786 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -1586,6 +1586,7 @@ class AggregateCursor : public Cursor { ExpressionEvaluator evaluator(&frame, context->symbol_table, context->evaluation_context, context->request_router, storage::v3::View::NEW); ProcessOne(frame, &evaluator); + frame.MakeInvalid(); } } From 41183b328b49d4c70e363da1480141818e217c54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 1 Feb 2023 17:19:02 +0100 Subject: [PATCH 08/56] Invalidate consumed frames in `ScanByPrimaryKey` --- src/query/v2/plan/operator.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index e24e4d786..4ed9e00e4 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -774,6 +774,7 @@ class DistributedScanByPrimaryKeyCursor : public Cursor { populated_any = true; ++output_frame_it; } + own_frames_it_->MakeInvalid(); } break; } From 2219dee6f606a00b0d9a5fe17b3984fda1b04547 Mon Sep 17 00:00:00 2001 From: gvolfing Date: Mon, 6 Feb 2023 12:49:32 +0100 Subject: [PATCH 09/56] Add initial impl for EdgeUniquenessFilterCursor::PullMultiple --- src/query/v2/interpreter.cpp | 3 +- src/query/v2/plan/operator.cpp | 126 +++++++++++++++++++++++++++++---- src/query/v2/plan/operator.lcp | 7 ++ 3 files changed, 122 insertions(+), 14 deletions(-) diff --git a/src/query/v2/interpreter.cpp b/src/query/v2/interpreter.cpp index aa220d764..40c3fced4 100644 --- a/src/query/v2/interpreter.cpp +++ b/src/query/v2/interpreter.cpp @@ -812,7 +812,8 @@ std::optional PullPlan::PullMultiple(AnyStrea std::optional PullPlan::Pull(AnyStream *stream, std::optional n, const std::vector &output_symbols, std::map *summary) { - auto should_pull_multiple = false; // TODO on the long term, we will only use PullMultiple + // auto should_pull_multiple = false; // TODO on the long term, we will only use PullMultiple + auto should_pull_multiple = false; if (should_pull_multiple) { return PullMultiple(stream, n, output_symbols, summary); } diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index aa9743878..8bd3544cf 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -1207,28 +1207,128 @@ bool ContainsSameEdge(const TypedValue &a, const TypedValue &b) { return a.ValueEdge() == b.ValueEdge(); } + +bool IsExpansionOk(Frame &frame, const Symbol &expand_symbol, const std::vector &previous_symbols) { + const auto &expand_value = frame[expand_symbol]; + for (const auto &previous_symbol : previous_symbols) { + const auto &previous_value = frame[previous_symbol]; + // This shouldn't raise a TypedValueException, because the planner + // makes sure these are all of the expected type. In case they are not + // an error should be raised long before this code is executed. + if (ContainsSameEdge(previous_value, expand_value)) return false; + } + return true; +} + } // namespace bool EdgeUniquenessFilter::EdgeUniquenessFilterCursor::Pull(Frame &frame, ExecutionContext &context) { SCOPED_PROFILE_OP("EdgeUniquenessFilter"); - - auto expansion_ok = [&]() { - const auto &expand_value = frame[self_.expand_symbol_]; - for (const auto &previous_symbol : self_.previous_symbols_) { - const auto &previous_value = frame[previous_symbol]; - // This shouldn't raise a TypedValueException, because the planner - // makes sure these are all of the expected type. In case they are not - // an error should be raised long before this code is executed. - if (ContainsSameEdge(previous_value, expand_value)) return false; - } - return true; - }; + // // TODO (gvolfing) Make the simple Pull method use the function instead of the lambda as well. + // auto expansion_ok = [&]() { + // const auto &expand_value = frame[self_.expand_symbol_]; + // for (const auto &previous_symbol : self_.previous_symbols_) { + // const auto &previous_value = frame[previous_symbol]; + // // This shouldn't raise a TypedValueException, because the planner + // // makes sure these are all of the expected type. In case they are not + // // an error should be raised long before this code is executed. + // if (ContainsSameEdge(previous_value, expand_value)) return false; + // } + // return true; + // }; while (input_cursor_->Pull(frame, context)) - if (expansion_ok()) return true; + if (IsExpansionOk(frame, self_.expand_symbol_, self_.previous_symbols_)) return true; return false; } +bool EdgeUniquenessFilter::EdgeUniquenessFilterCursor::PullMultiple(MultiFrame &output_multi_frame, + ExecutionContext &context) { + SCOPED_PROFILE_OP("EdgeUniquenessFilterMF"); + + if (!own_multi_frame_.has_value()) { + own_multi_frame_.emplace(MultiFrame(output_multi_frame.GetFirstFrame().elems().size(), kNumberOfFramesInMultiframe, + output_multi_frame.GetMemoryResource())); + own_frames_consumer_.emplace(own_multi_frame_->GetValidFramesConsumer()); + own_frames_it_ = own_frames_consumer_->begin(); + } + MG_ASSERT(output_multi_frame.GetFirstFrame().elems().size() == own_multi_frame_->GetFirstFrame().elems().size()); + + auto output_frames_populator = output_multi_frame.GetInvalidFramesPopulator(); + auto populated_any = false; + + while (true) { + switch (state_) { + case State::PullInput: { + if (!input_cursor_->PullMultiple(*own_multi_frame_, context)) { + state_ = State::Exhausted; + return populated_any; + } + own_frames_consumer_.emplace(own_multi_frame_->GetValidFramesConsumer()); + own_frames_it_ = own_frames_consumer_->begin(); + + if (own_frames_it_ == own_frames_consumer_->end()) { + continue; + } + + state_ = State::PopulateOutput; + break; + } + case State::PopulateOutput: { + if (!output_multi_frame.HasInvalidFrame()) { + return populated_any; + } + + if (own_frames_it_ == own_frames_consumer_->end()) { + state_ = State::PullInput; + continue; + } + + for (auto output_frame_it = output_frames_populator.begin(); + output_frame_it != output_frames_populator.end() && own_frames_it_ != own_frames_consumer_->end(); + ++own_frames_it_) { + auto &output_frame = *output_frame_it; + + if (IsExpansionOk(*own_frames_it_, self_.expand_symbol_, self_.previous_symbols_)) { + output_frame = *own_frames_it_; + populated_any = true; + } else { + own_frames_it_->MakeInvalid(); + } + ++output_frame_it; + + ///////////////////////////////////////////////// + /* + ExpressionEvaluator evaluator(&*own_frames_it_, context.symbol_table, context.evaluation_context, + context.request_router, storage::v3::View::NEW); + + std::vector pk; + for (auto *primary_property : primary_key_) { + pk.push_back(TypedValueToValue(primary_property->Accept(evaluator))); + } + + const msgs::Label label = {.id = msgs::LabelId::FromUint(label_.AsUint())}; + auto vertex_id = std::make_pair(label, std::move(pk)); + + if (const auto it = id_to_accessor_mapping_.find(vertex_id); it != id_to_accessor_mapping_.end()) { + output_frame = *own_frames_it_; + output_frame[output_symbol_] = TypedValue(it->second); + populated_any = true; + ++output_frame_it; + } + own_frames_it_->MakeInvalid(); + */ + } + break; + } + case State::Exhausted: { + return populated_any; + } + } + } + return populated_any; +} + void EdgeUniquenessFilter::EdgeUniquenessFilterCursor::Shutdown() { input_cursor_->Shutdown(); } void EdgeUniquenessFilter::EdgeUniquenessFilterCursor::Reset() { input_cursor_->Reset(); } diff --git a/src/query/v2/plan/operator.lcp b/src/query/v2/plan/operator.lcp index 4f34cc061..ae02b5931 100644 --- a/src/query/v2/plan/operator.lcp +++ b/src/query/v2/plan/operator.lcp @@ -1570,12 +1570,19 @@ edge lists).") EdgeUniquenessFilterCursor(const EdgeUniquenessFilter &, utils::MemoryResource *); bool Pull(Frame &, ExecutionContext &) override; + bool PullMultiple(MultiFrame &, ExecutionContext &) override; void Shutdown() override; void Reset() override; private: const EdgeUniquenessFilter &self_; const UniqueCursorPtr input_cursor_; + enum class State { PullInput, PopulateOutput, Exhausted }; + + State state_{State::PullInput}; + std::optional own_multi_frame_; + std::optional own_frames_consumer_; + ValidFramesConsumer::Iterator own_frames_it_; }; cpp<#) (:serialize (:slk)) From 7e99f32adb080bea62149d4ec200a5b7ea479236 Mon Sep 17 00:00:00 2001 From: gvolfing Date: Mon, 6 Feb 2023 15:47:18 +0100 Subject: [PATCH 10/56] Remove uncommented, useless code --- src/query/v2/interpreter.cpp | 3 +-- src/query/v2/plan/operator.cpp | 37 +--------------------------------- 2 files changed, 2 insertions(+), 38 deletions(-) diff --git a/src/query/v2/interpreter.cpp b/src/query/v2/interpreter.cpp index 40c3fced4..aa220d764 100644 --- a/src/query/v2/interpreter.cpp +++ b/src/query/v2/interpreter.cpp @@ -812,8 +812,7 @@ std::optional PullPlan::PullMultiple(AnyStrea std::optional PullPlan::Pull(AnyStream *stream, std::optional n, const std::vector &output_symbols, std::map *summary) { - // auto should_pull_multiple = false; // TODO on the long term, we will only use PullMultiple - auto should_pull_multiple = false; + auto should_pull_multiple = false; // TODO on the long term, we will only use PullMultiple if (should_pull_multiple) { return PullMultiple(stream, n, output_symbols, summary); } diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index 5397d6735..beca89b13 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -761,7 +761,7 @@ class DistributedScanByPrimaryKeyCursor : public Cursor { output_frame[output_symbol_] = TypedValue(it->second); populated_any = true; ++output_frame_it; - } + } own_frames_it_->MakeInvalid(); } break; @@ -1350,19 +1350,6 @@ bool IsExpansionOk(Frame &frame, const Symbol &expand_symbol, const std::vector< bool EdgeUniquenessFilter::EdgeUniquenessFilterCursor::Pull(Frame &frame, ExecutionContext &context) { SCOPED_PROFILE_OP("EdgeUniquenessFilter"); - // // TODO (gvolfing) Make the simple Pull method use the function instead of the lambda as well. - // auto expansion_ok = [&]() { - // const auto &expand_value = frame[self_.expand_symbol_]; - // for (const auto &previous_symbol : self_.previous_symbols_) { - // const auto &previous_value = frame[previous_symbol]; - // // This shouldn't raise a TypedValueException, because the planner - // // makes sure these are all of the expected type. In case they are not - // // an error should be raised long before this code is executed. - // if (ContainsSameEdge(previous_value, expand_value)) return false; - // } - // return true; - // }; - while (input_cursor_->Pull(frame, context)) if (IsExpansionOk(frame, self_.expand_symbol_, self_.previous_symbols_)) return true; return false; @@ -1422,28 +1409,6 @@ bool EdgeUniquenessFilter::EdgeUniquenessFilterCursor::PullMultiple(MultiFrame & own_frames_it_->MakeInvalid(); } ++output_frame_it; - - ///////////////////////////////////////////////// - /* - ExpressionEvaluator evaluator(&*own_frames_it_, context.symbol_table, context.evaluation_context, - context.request_router, storage::v3::View::NEW); - - std::vector pk; - for (auto *primary_property : primary_key_) { - pk.push_back(TypedValueToValue(primary_property->Accept(evaluator))); - } - - const msgs::Label label = {.id = msgs::LabelId::FromUint(label_.AsUint())}; - auto vertex_id = std::make_pair(label, std::move(pk)); - - if (const auto it = id_to_accessor_mapping_.find(vertex_id); it != id_to_accessor_mapping_.end()) { - output_frame = *own_frames_it_; - output_frame[output_symbol_] = TypedValue(it->second); - populated_any = true; - ++output_frame_it; - } - own_frames_it_->MakeInvalid(); - */ } break; } From ac59e7f7e092fd357926f5faf5d845e8af19ca2f Mon Sep 17 00:00:00 2001 From: gvolfing Date: Mon, 6 Feb 2023 15:54:07 +0100 Subject: [PATCH 11/56] Move loop variables incrementation into the same place --- src/query/v2/plan/operator.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index beca89b13..400ee4f53 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -1399,7 +1399,7 @@ bool EdgeUniquenessFilter::EdgeUniquenessFilterCursor::PullMultiple(MultiFrame & for (auto output_frame_it = output_frames_populator.begin(); output_frame_it != output_frames_populator.end() && own_frames_it_ != own_frames_consumer_->end(); - ++own_frames_it_) { + ++own_frames_it_, ++output_frame_it) { auto &output_frame = *output_frame_it; if (IsExpansionOk(*own_frames_it_, self_.expand_symbol_, self_.previous_symbols_)) { @@ -1408,7 +1408,6 @@ bool EdgeUniquenessFilter::EdgeUniquenessFilterCursor::PullMultiple(MultiFrame & } else { own_frames_it_->MakeInvalid(); } - ++output_frame_it; } break; } From 2b01f2280c6b42408fdd0418251babd77d9e03b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Mon, 6 Feb 2023 16:14:21 +0100 Subject: [PATCH 12/56] Add `TODO` about failing query --- src/query/v2/plan/operator.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index c1df73549..d53c2f0c9 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -904,6 +904,7 @@ ScanByPrimaryKey::ScanByPrimaryKey(const std::shared_ptr &input storage::v3::LabelId label, std::vector primary_key, storage::v3::View view) : ScanAll(input, output_symbol, view), label_(label), primary_key_(primary_key) { + // TODO(antaljanosbenjamin): MATCH (p:Permission) WHERE p.uuid <999 RETURN p; MG_ASSERT(primary_key.front()); } From 4bad8c0d1e7e03b093bb15627d7cac43d3d75efd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Mon, 6 Feb 2023 16:14:39 +0100 Subject: [PATCH 13/56] Filter edges on types --- src/query/v2/plan/operator.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index d53c2f0c9..c908abb59 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -3105,6 +3105,9 @@ class DistributedExpandCursor : public Cursor { auto &vertex = vertex_value.ValueVertex(); msgs::ExpandOneRequest request; request.direction = DirectionToMsgsDirection(self_.common_.direction); + std::transform(self_.common_.edge_types.begin(), self_.common_.edge_types.end(), + std::back_inserter(request.edge_types), + [](const storage::v3::EdgeTypeId edge_type_id) { return msgs::EdgeType{edge_type_id}; }); // to not fetch any properties of the edges request.edge_properties.emplace(); request.src_vertices.push_back(vertex.Id()); @@ -3245,6 +3248,9 @@ class DistributedExpandCursor : public Cursor { msgs::ExpandOneRequest request; request.direction = DirectionToMsgsDirection(self_.common_.direction); + std::transform(self_.common_.edge_types.begin(), self_.common_.edge_types.end(), + std::back_inserter(request.edge_types), + [](const storage::v3::EdgeTypeId edge_type_id) { return msgs::EdgeType{edge_type_id}; }); // to not fetch any properties of the edges request.edge_properties.emplace(); for (const auto &frame : own_multi_frame_->GetValidFramesReader()) { From b26c7d09ef640d80c2c37342a558717676290a7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Mon, 6 Feb 2023 16:39:42 +0100 Subject: [PATCH 14/56] Ignore not value equality property filters for `ScanByPrimaryKey` --- src/query/v2/plan/operator.cpp | 1 - src/query/v2/plan/rewrite/index_lookup.hpp | 3 +++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index c908abb59..2ad88cfa9 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -904,7 +904,6 @@ ScanByPrimaryKey::ScanByPrimaryKey(const std::shared_ptr &input storage::v3::LabelId label, std::vector primary_key, storage::v3::View view) : ScanAll(input, output_symbol, view), label_(label), primary_key_(primary_key) { - // TODO(antaljanosbenjamin): MATCH (p:Permission) WHERE p.uuid <999 RETURN p; MG_ASSERT(primary_key.front()); } diff --git a/src/query/v2/plan/rewrite/index_lookup.hpp b/src/query/v2/plan/rewrite/index_lookup.hpp index 17996d952..0b9b9cb97 100644 --- a/src/query/v2/plan/rewrite/index_lookup.hpp +++ b/src/query/v2/plan/rewrite/index_lookup.hpp @@ -597,6 +597,9 @@ class IndexLookupRewriter final : public HierarchicalLogicalOperatorVisitor { [](const auto &schema_elem) { return schema_elem.property_id; }); for (const auto &property_filter : property_filters) { + if (property_filter.property_filter->type_ != PropertyFilter::Type::EQUAL) { + continue; + } const auto &property_id = db_->NameToProperty(property_filter.property_filter->property_.name); if (std::find(schema_properties.begin(), schema_properties.end(), property_id) != schema_properties.end()) { pk_temp.emplace_back(std::make_pair(property_filter.expression, property_filter)); From 37f19867b088ee2a06cdafe36341eb84a5af37f2 Mon Sep 17 00:00:00 2001 From: gvolfing Date: Tue, 7 Feb 2023 08:25:50 +0100 Subject: [PATCH 15/56] Make EdgeUniquenessFilterCursor impl simpler --- src/query/v2/plan/operator.cpp | 62 +++++----------------------------- src/query/v2/plan/operator.lcp | 6 ---- 2 files changed, 9 insertions(+), 59 deletions(-) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index 400ee4f53..ec5413981 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -1358,61 +1358,17 @@ bool EdgeUniquenessFilter::EdgeUniquenessFilterCursor::Pull(Frame &frame, Execut bool EdgeUniquenessFilter::EdgeUniquenessFilterCursor::PullMultiple(MultiFrame &output_multi_frame, ExecutionContext &context) { SCOPED_PROFILE_OP("EdgeUniquenessFilterMF"); - - if (!own_multi_frame_.has_value()) { - own_multi_frame_.emplace(MultiFrame(output_multi_frame.GetFirstFrame().elems().size(), kNumberOfFramesInMultiframe, - output_multi_frame.GetMemoryResource())); - own_frames_consumer_.emplace(own_multi_frame_->GetValidFramesConsumer()); - own_frames_it_ = own_frames_consumer_->begin(); - } - MG_ASSERT(output_multi_frame.GetFirstFrame().elems().size() == own_multi_frame_->GetFirstFrame().elems().size()); - - auto output_frames_populator = output_multi_frame.GetInvalidFramesPopulator(); auto populated_any = false; - while (true) { - switch (state_) { - case State::PullInput: { - if (!input_cursor_->PullMultiple(*own_multi_frame_, context)) { - state_ = State::Exhausted; - return populated_any; - } - own_frames_consumer_.emplace(own_multi_frame_->GetValidFramesConsumer()); - own_frames_it_ = own_frames_consumer_->begin(); - - if (own_frames_it_ == own_frames_consumer_->end()) { - continue; - } - - state_ = State::PopulateOutput; - break; - } - case State::PopulateOutput: { - if (!output_multi_frame.HasInvalidFrame()) { - return populated_any; - } - - if (own_frames_it_ == own_frames_consumer_->end()) { - state_ = State::PullInput; - continue; - } - - for (auto output_frame_it = output_frames_populator.begin(); - output_frame_it != output_frames_populator.end() && own_frames_it_ != own_frames_consumer_->end(); - ++own_frames_it_, ++output_frame_it) { - auto &output_frame = *output_frame_it; - - if (IsExpansionOk(*own_frames_it_, self_.expand_symbol_, self_.previous_symbols_)) { - output_frame = *own_frames_it_; - populated_any = true; - } else { - own_frames_it_->MakeInvalid(); - } - } - break; - } - case State::Exhausted: { - return populated_any; + while (output_multi_frame.HasInvalidFrame()) { + if (!input_cursor_->PullMultiple(output_multi_frame, context)) { + return populated_any; + } + for (auto &frame : output_multi_frame.GetValidFramesConsumer()) { + if (IsExpansionOk(frame, self_.expand_symbol_, self_.previous_symbols_)) { + populated_any = true; + } else { + frame.MakeInvalid(); } } } diff --git a/src/query/v2/plan/operator.lcp b/src/query/v2/plan/operator.lcp index ae02b5931..110ba8a33 100644 --- a/src/query/v2/plan/operator.lcp +++ b/src/query/v2/plan/operator.lcp @@ -1577,12 +1577,6 @@ edge lists).") private: const EdgeUniquenessFilter &self_; const UniqueCursorPtr input_cursor_; - enum class State { PullInput, PopulateOutput, Exhausted }; - - State state_{State::PullInput}; - std::optional own_multi_frame_; - std::optional own_frames_consumer_; - ValidFramesConsumer::Iterator own_frames_it_; }; cpp<#) (:serialize (:slk)) From 292a55f4ff59aa6e4a45569bfed650ac6e60bf3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 8 Feb 2023 11:28:19 +0100 Subject: [PATCH 16/56] Add new line at the end of dataset file --- tests/mgbench/dataset_creator_unwind.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/mgbench/dataset_creator_unwind.py b/tests/mgbench/dataset_creator_unwind.py index 84434ba6f..564a4d018 100644 --- a/tests/mgbench/dataset_creator_unwind.py +++ b/tests/mgbench/dataset_creator_unwind.py @@ -131,7 +131,7 @@ def main(): ) created = 0 f.write( - "] AS props MATCH (file:File {uuid:props.fileUuid}), (identity:Identity {uuid: props.identityUuid}) CREATE (permission:Permission {uuid: props.permUuid, name: props.permName}) CREATE (permission)-[: IS_FOR_FILE]->(file) CREATE (permission)-[: IS_FOR_IDENTITY]->(identity);" + "] AS props MATCH (file:File {uuid:props.fileUuid}), (identity:Identity {uuid: props.identityUuid}) CREATE (permission:Permission {uuid: props.permUuid, name: props.permName}) CREATE (permission)-[: IS_FOR_FILE]->(file) CREATE (permission)-[: IS_FOR_IDENTITY]->(identity);\n" ) From 25226cca920d8c93f603713909f8b266da4623f9 Mon Sep 17 00:00:00 2001 From: gvolfing <107616712+gvolfing@users.noreply.github.com> Date: Wed, 8 Feb 2023 11:41:43 +0100 Subject: [PATCH 17/56] Update src/query/v2/plan/operator.cpp Co-authored-by: Jure Bajic --- src/query/v2/plan/operator.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index ec5413981..6fa9c4db9 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -1335,15 +1335,12 @@ bool ContainsSameEdge(const TypedValue &a, const TypedValue &b) { } bool IsExpansionOk(Frame &frame, const Symbol &expand_symbol, const std::vector &previous_symbols) { - const auto &expand_value = frame[expand_symbol]; - for (const auto &previous_symbol : previous_symbols) { - const auto &previous_value = frame[previous_symbol]; // This shouldn't raise a TypedValueException, because the planner // makes sure these are all of the expected type. In case they are not // an error should be raised long before this code is executed. - if (ContainsSameEdge(previous_value, expand_value)) return false; - } - return true; + return std::ranges::all_of(previous_symbols, [&expand_value = frame[expand_symbol]](const auto& previous_symbol) { + return ContainsSameEdge(previous_value, expand_value); + }); } } // namespace From 657279949aece28624900b012365adca8fa9d9a2 Mon Sep 17 00:00:00 2001 From: gvolfing Date: Wed, 8 Feb 2023 12:13:46 +0100 Subject: [PATCH 18/56] Fix compile error --- src/query/v2/plan/operator.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index 6fa9c4db9..4337add0d 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -1335,12 +1335,14 @@ bool ContainsSameEdge(const TypedValue &a, const TypedValue &b) { } bool IsExpansionOk(Frame &frame, const Symbol &expand_symbol, const std::vector &previous_symbols) { - // This shouldn't raise a TypedValueException, because the planner - // makes sure these are all of the expected type. In case they are not - // an error should be raised long before this code is executed. - return std::ranges::all_of(previous_symbols, [&expand_value = frame[expand_symbol]](const auto& previous_symbol) { - return ContainsSameEdge(previous_value, expand_value); - }); + // This shouldn't raise a TypedValueException, because the planner + // makes sure these are all of the expected type. In case they are not + // an error should be raised long before this code is executed. + return std::ranges::all_of(previous_symbols, + [&frame, &expand_value = frame[expand_symbol]](const auto &previous_symbol) { + const auto &previous_value = frame[previous_symbol]; + return ContainsSameEdge(previous_value, expand_value); + }); } } // namespace From 096d1ce5f4d4e609614c3795fb22265abb692408 Mon Sep 17 00:00:00 2001 From: gvolfing Date: Wed, 8 Feb 2023 12:57:21 +0100 Subject: [PATCH 19/56] Invert boolean logic when checking for unique edges --- src/query/v2/plan/operator.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index 4337add0d..73de9ee28 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -1341,7 +1341,7 @@ bool IsExpansionOk(Frame &frame, const Symbol &expand_symbol, const std::vector< return std::ranges::all_of(previous_symbols, [&frame, &expand_value = frame[expand_symbol]](const auto &previous_symbol) { const auto &previous_value = frame[previous_symbol]; - return ContainsSameEdge(previous_value, expand_value); + return !ContainsSameEdge(previous_value, expand_value); }); } From a9a388ce44affe027f27f33bea5fc9cdf16398ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 8 Feb 2023 13:52:51 +0100 Subject: [PATCH 20/56] Use parametrized queries for vertex creation --- tests/mgbench/datasets.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/mgbench/datasets.py b/tests/mgbench/datasets.py index 3a5806629..83ae28a2e 100644 --- a/tests/mgbench/datasets.py +++ b/tests/mgbench/datasets.py @@ -353,7 +353,7 @@ class AccessControl(Dataset): def benchmark__create__vertex(self): self.next_value_idx += 1 - query = (f"CREATE (:File {{uuid: {self.next_value_idx}}});", {}) + query = ("CREATE (:File {uuid: $uuid})", {"uuid": self.next_value_idx}) return query def benchmark__create__edges(self): From 12bc78ca2d663eac4cd84f9d6eb09572d090ff67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Thu, 9 Feb 2023 12:41:17 +0100 Subject: [PATCH 21/56] Add command line flag to determine `MultiFrame` size --- src/query/v2/interpreter.cpp | 2 +- src/query/v2/multiframe.cpp | 2 ++ src/query/v2/multiframe.hpp | 5 ++++- src/query/v2/plan/operator.cpp | 8 ++++---- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/query/v2/interpreter.cpp b/src/query/v2/interpreter.cpp index aa220d764..6f07b598e 100644 --- a/src/query/v2/interpreter.cpp +++ b/src/query/v2/interpreter.cpp @@ -688,7 +688,7 @@ PullPlan::PullPlan(const std::shared_ptr plan, const Parameters &par : plan_(plan), cursor_(plan->plan().MakeCursor(execution_memory)), frame_(plan->symbol_table().max_position(), execution_memory), - multi_frame_(plan->symbol_table().max_position(), kNumberOfFramesInMultiframe, execution_memory), + multi_frame_(plan->symbol_table().max_position(), FLAGS_default_multi_frame_size, execution_memory), memory_limit_(memory_limit) { ctx_.db_accessor = dba; ctx_.symbol_table = plan->symbol_table(); diff --git a/src/query/v2/multiframe.cpp b/src/query/v2/multiframe.cpp index 477ef6c0c..264990bb7 100644 --- a/src/query/v2/multiframe.cpp +++ b/src/query/v2/multiframe.cpp @@ -17,6 +17,8 @@ #include "query/v2/bindings/frame.hpp" #include "utils/pmr/vector.hpp" +DEFINE_uint64(default_multi_frame_size, 100, "Default size of MultiFrame"); + namespace memgraph::query::v2 { static_assert(std::forward_iterator); diff --git a/src/query/v2/multiframe.hpp b/src/query/v2/multiframe.hpp index 6958ffbe8..396092c2b 100644 --- a/src/query/v2/multiframe.hpp +++ b/src/query/v2/multiframe.hpp @@ -13,10 +13,13 @@ #include +#include + #include "query/v2/bindings/frame.hpp" +DECLARE_uint64(default_multi_frame_size); + namespace memgraph::query::v2 { -constexpr uint64_t kNumberOfFramesInMultiframe = 1000; // TODO have it configurable class ValidFramesConsumer; class ValidFramesModifier; diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index 1841668bb..993d24282 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -509,7 +509,7 @@ class DistributedScanAllAndFilterCursor : public Cursor { if (!own_multi_frame_.has_value()) { own_multi_frame_.emplace(MultiFrame(output_multi_frame.GetFirstFrame().elems().size(), - kNumberOfFramesInMultiframe, output_multi_frame.GetMemoryResource())); + FLAGS_default_multi_frame_size, output_multi_frame.GetMemoryResource())); own_frames_consumer_.emplace(own_multi_frame_->GetValidFramesConsumer()); own_frames_it_ = own_frames_consumer_->begin(); } @@ -705,7 +705,7 @@ class DistributedScanByPrimaryKeyCursor : public Cursor { void EnsureOwnMultiFrameIsGood(MultiFrame &output_multi_frame) { if (!own_multi_frame_.has_value()) { own_multi_frame_.emplace(MultiFrame(output_multi_frame.GetFirstFrame().elems().size(), - kNumberOfFramesInMultiframe, output_multi_frame.GetMemoryResource())); + FLAGS_default_multi_frame_size, output_multi_frame.GetMemoryResource())); own_frames_consumer_.emplace(own_multi_frame_->GetValidFramesConsumer()); own_frames_it_ = own_frames_consumer_->begin(); } @@ -2213,7 +2213,7 @@ class UnwindCursor : public Cursor { if (!own_multi_frame_.has_value()) { own_multi_frame_.emplace(MultiFrame(output_multi_frame.GetFirstFrame().elems().size(), - kNumberOfFramesInMultiframe, output_multi_frame.GetMemoryResource())); + FLAGS_default_multi_frame_size, output_multi_frame.GetMemoryResource())); own_frames_consumer_.emplace(own_multi_frame_->GetValidFramesConsumer()); own_frames_it_ = own_frames_consumer_->begin(); } @@ -3382,7 +3382,7 @@ class DistributedExpandCursor : public Cursor { void EnsureOwnMultiFrameIsGood(MultiFrame &output_multi_frame) { if (!own_multi_frame_.has_value()) { own_multi_frame_.emplace(MultiFrame(output_multi_frame.GetFirstFrame().elems().size(), - kNumberOfFramesInMultiframe, output_multi_frame.GetMemoryResource())); + FLAGS_default_multi_frame_size, output_multi_frame.GetMemoryResource())); own_frames_consumer_.emplace(own_multi_frame_->GetValidFramesConsumer()); own_frames_it_ = own_frames_consumer_->begin(); } From 563035645cb5859f51ab4f667885213858eaaa45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Thu, 9 Feb 2023 12:41:41 +0100 Subject: [PATCH 22/56] Add command line flag to opt for using `MultiFrame` --- src/query/v2/interpreter.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/query/v2/interpreter.cpp b/src/query/v2/interpreter.cpp index 6f07b598e..d85297909 100644 --- a/src/query/v2/interpreter.cpp +++ b/src/query/v2/interpreter.cpp @@ -64,6 +64,8 @@ #include "utils/tsc.hpp" #include "utils/variant_helpers.hpp" +DEFINE_bool(use_multi_frame, false, "Whether to use MultiFrame or not"); + namespace EventCounter { extern Event ReadQuery; extern Event WriteQuery; @@ -812,8 +814,7 @@ std::optional PullPlan::PullMultiple(AnyStrea std::optional PullPlan::Pull(AnyStream *stream, std::optional n, const std::vector &output_symbols, std::map *summary) { - auto should_pull_multiple = false; // TODO on the long term, we will only use PullMultiple - if (should_pull_multiple) { + if (FLAGS_use_multi_frame) { return PullMultiple(stream, n, output_symbols, summary); } // Set up temporary memory for a single Pull. Initial memory comes from the From b678e6a63b15a8255465f9d0627fbf75805ef979 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Thu, 9 Feb 2023 12:42:02 +0100 Subject: [PATCH 23/56] Handle bool flags properly in benchmark runner --- tests/mgbench/runners.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/tests/mgbench/runners.py b/tests/mgbench/runners.py index 2b69a811f..cf89d7f67 100644 --- a/tests/mgbench/runners.py +++ b/tests/mgbench/runners.py @@ -68,6 +68,15 @@ class Memgraph: self._cleanup() atexit.unregister(self._cleanup) + # Returns None if string_value is not true or false, casing doesn't matter + def _get_bool_value(self, string_value): + lower_string_value = string_value.lower() + if lower_string_value == "true": + return True + if lower_string_value == "false": + return False + return None + def _get_args(self, **kwargs): data_directory = os.path.join(self._directory.name, "memgraph") if self._memgraph_version >= (0, 50, 0): @@ -83,7 +92,13 @@ class Memgraph: args_list = self._extra_args.split(" ") assert len(args_list) % 2 == 0 for i in range(0, len(args_list), 2): - kwargs[args_list[i]] = args_list[i + 1] + key = args_list[i] + value = args_list[i + 1] + maybe_bool_value = self._get_bool_value(value) + if maybe_bool_value is not None: + kwargs[key] = maybe_bool_value + else: + kwargs[key] = value return _convert_args_to_flags(self._memgraph_binary, **kwargs) From 53f95ed1a7c0313928554c803ca97a7843652517 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Thu, 9 Feb 2023 13:17:15 +0100 Subject: [PATCH 24/56] Format python file --- tests/mgbench/compare_results.py | 65 ++++++++++++++------------------ 1 file changed, 29 insertions(+), 36 deletions(-) diff --git a/tests/mgbench/compare_results.py b/tests/mgbench/compare_results.py index 2179bb408..f10ec8bab 100755 --- a/tests/mgbench/compare_results.py +++ b/tests/mgbench/compare_results.py @@ -14,7 +14,6 @@ import argparse import json - FIELDS = [ { "name": "throughput", @@ -85,39 +84,32 @@ def compare_results(results_from, results_to, fields): if group == "__import__": continue for scenario, summary_to in scenarios.items(): - summary_from = recursive_get( - results_from, dataset, variant, group, scenario, - value={}) - if len(summary_from) > 0 and \ - summary_to["count"] != summary_from["count"] or \ - summary_to["num_workers"] != \ - summary_from["num_workers"]: + summary_from = recursive_get(results_from, dataset, variant, group, scenario, value={}) + if ( + len(summary_from) > 0 + and summary_to["count"] != summary_from["count"] + or summary_to["num_workers"] != summary_from["num_workers"] + ): raise Exception("Incompatible results!") - testcode = "/".join([dataset, variant, group, scenario, - "{:02d}".format( - summary_to["num_workers"])]) + testcode = "/".join([dataset, variant, group, scenario, "{:02d}".format(summary_to["num_workers"])]) row = {} performance_changed = False for field in fields: key = field["name"] if key in summary_to: - row[key] = compute_diff( - summary_from.get(key, None), - summary_to[key]) + row[key] = compute_diff(summary_from.get(key, None), summary_to[key]) elif key in summary_to["database"]: row[key] = compute_diff( - recursive_get(summary_from, "database", key, - value=None), - summary_to["database"][key]) + recursive_get(summary_from, "database", key, value=None), summary_to["database"][key] + ) else: row[key] = compute_diff( - recursive_get(summary_from, "metadata", key, - "average", value=None), - summary_to["metadata"][key]["average"]) - if "diff" not in row[key] or \ - ("diff_treshold" in field and - abs(row[key]["diff"]) >= - field["diff_treshold"]): + recursive_get(summary_from, "metadata", key, "average", value=None), + summary_to["metadata"][key]["average"], + ) + if "diff" not in row[key] or ( + "diff_treshold" in field and abs(row[key]["diff"]) >= field["diff_treshold"] + ): performance_changed = True if performance_changed: ret[testcode] = row @@ -130,8 +122,9 @@ def generate_remarkup(fields, data): ret += "\n" ret += " \n" ret += " \n" - ret += "\n".join(map(lambda x: " ".format( - x["name"].replace("_", " ").capitalize()), fields)) + "\n" + ret += ( + "\n".join(map(lambda x: " ".format(x["name"].replace("_", " ").capitalize()), fields)) + "\n" + ) ret += " \n" for testcode in sorted(data.keys()): ret += " \n" @@ -147,12 +140,9 @@ def generate_remarkup(fields, data): else: color = "red" sign = "{{icon {} color={}}}".format(arrow, color) - ret += " \n".format( - value, field["unit"], diff, sign) + ret += " \n".format(value, field["unit"], diff, sign) else: - ret += " \n".format( - value, field["unit"]) + ret += " \n".format(value, field["unit"]) ret += " \n" ret += "
Testcode{}{}
{:.3f}{} //({:+.2%})// {}{:.3f}{} //({:+.2%})// {}{:.3f}{} //(new)// " \ - "{{icon plus color=blue}}{:.3f}{} //(new)// " "{{icon plus color=blue}}
\n" else: @@ -161,11 +151,14 @@ def generate_remarkup(fields, data): if __name__ == "__main__": - parser = argparse.ArgumentParser( - description="Compare results of multiple benchmark runs.") - parser.add_argument("--compare", action="append", nargs=2, - metavar=("from", "to"), - help="compare results between `from` and `to` files") + parser = argparse.ArgumentParser(description="Compare results of multiple benchmark runs.") + parser.add_argument( + "--compare", + action="append", + nargs=2, + metavar=("from", "to"), + help="compare results between `from` and `to` files", + ) parser.add_argument("--output", default="", help="output file name") args = parser.parse_args() From 2b3141879bc84dac2c37b430bbca9eb58e014bd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Thu, 9 Feb 2023 13:17:46 +0100 Subject: [PATCH 25/56] Make the output table nicer for comparing results --- tests/mgbench/compare_results.py | 35 ++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/tests/mgbench/compare_results.py b/tests/mgbench/compare_results.py index f10ec8bab..46da74270 100755 --- a/tests/mgbench/compare_results.py +++ b/tests/mgbench/compare_results.py @@ -123,26 +123,35 @@ def generate_remarkup(fields, data): ret += " \n" ret += " Testcode\n" ret += ( - "\n".join(map(lambda x: " {}".format(x["name"].replace("_", " ").capitalize()), fields)) + "\n" + "\n".join( + map( + lambda x: " {}".format(x["name"].replace("_", " ").capitalize()), + fields, + ) + ) + + "\n" ) ret += " \n" for testcode in sorted(data.keys()): ret += " \n" ret += " {}\n".format(testcode) for field in fields: - result = data[testcode][field["name"]] - value = result["value"] * field["scaling"] - if "diff" in result: - diff = result["diff"] - arrow = "arrow-up" if diff >= 0 else "arrow-down" - if not (field["positive_diff_better"] ^ (diff >= 0)): - color = "green" + result = data[testcode].get(field["name"]) + if result != None: + value = result["value"] * field["scaling"] + if "diff" in result: + diff = result["diff"] + arrow = "arrow-up" if diff >= 0 else "arrow-down" + if not (field["positive_diff_better"] ^ (diff >= 0)): + color = "green" + else: + color = "red" + sign = "{{icon {} color={}}}".format(arrow, color) + ret += ' {:.3f}{} ({:+.2%})\n'.format( + color, value, field["unit"], diff + ) else: - color = "red" - sign = "{{icon {} color={}}}".format(arrow, color) - ret += " {:.3f}{} //({:+.2%})// {}\n".format(value, field["unit"], diff, sign) - else: - ret += " {:.3f}{} //(new)// " "{{icon plus color=blue}}\n".format(value, field["unit"]) + ret += '{:.3f}{} //(new)// \n'.format(value, field["unit"]) ret += " \n" ret += "\n" else: From 74f53369c063c993812b55c82c633b73244bb3b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Thu, 9 Feb 2023 12:44:34 +0100 Subject: [PATCH 26/56] Add two more queries to simple benchmark --- tests/mgbench/datasets.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/tests/mgbench/datasets.py b/tests/mgbench/datasets.py index 83ae28a2e..319b67c17 100644 --- a/tests/mgbench/datasets.py +++ b/tests/mgbench/datasets.py @@ -379,6 +379,24 @@ class AccessControl(Dataset): return query def benchmark__match__match_all_vertices_with_edges(self): - self.next_value_idx += 1 query = ("MATCH (permission:Permission)-[e:IS_FOR_FILE]->(file:File) RETURN *", {}) return query + + def benchmark__match__match_users_with_permission_for_files(self): + file_uuid_1 = self._get_random_uuid("File") + file_uuid_2 = self._get_random_uuid("File") + min_file_uuid = min(file_uuid_1, file_uuid_2) + max_file_uuid = max(file_uuid_1, file_uuid_2) + query = ( + "MATCH (f:File)<-[ff:IS_FOR_FILE]-(p:Permission)-[fi:IS_FOR_IDENTITY]->(i:Identity) WHERE f.uuid >= $min_file_uuid AND f.uuid <= $max_file_uuid RETURN *", + {"min_file_uuid": min_file_uuid, "max_file_uuid": max_file_uuid}, + ) + return query + + def benchmark__match__match_users_with_permission_for_specific_file(self): + file_uuid = self._get_random_uuid("File") + query = ( + "MATCH (f:File {uuid: $file_uuid})<-[ff:IS_FOR_FILE]-(p:Permission)-[fi:IS_FOR_IDENTITY]->(i:Identity) RETURN *", + {"file_uuid": file_uuid}, + ) + return query From 155388c0a1e845df4787e94360cc13183cac90b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Thu, 9 Feb 2023 17:35:04 +0100 Subject: [PATCH 27/56] Add benchmarking executable --- tests/manual/CMakeLists.txt | 3 + tests/manual/query_performance.cpp | 147 +++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+) create mode 100644 tests/manual/query_performance.cpp diff --git a/tests/manual/CMakeLists.txt b/tests/manual/CMakeLists.txt index ea680105c..a0c5983fd 100644 --- a/tests/manual/CMakeLists.txt +++ b/tests/manual/CMakeLists.txt @@ -61,3 +61,6 @@ target_link_libraries(${test_prefix}ssl_client mg-communication) add_manual_test(ssl_server.cpp) target_link_libraries(${test_prefix}ssl_server mg-communication) + +add_manual_test(query_performance.cpp) +target_link_libraries(${test_prefix}query_performance mg-communication mg-utils mg-io mg-io-simulator mg-coordinator mg-query-v2 mg-storage-v3) diff --git a/tests/manual/query_performance.cpp b/tests/manual/query_performance.cpp new file mode 100644 index 000000000..5d550a788 --- /dev/null +++ b/tests/manual/query_performance.cpp @@ -0,0 +1,147 @@ +// Copyright 2023 Memgraph Ltd. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source +// License, and you may not use this file except in compliance with the Business Source License. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +#include +#include +#include + +#include +#include +#include + +#include "io/address.hpp" +#include "io/local_transport/local_system.hpp" +#include "io/message_histogram_collector.hpp" +#include "machine_manager/machine_manager.hpp" +#include "query/v2/discard_value_stream.hpp" +#include "query/v2/interpreter.hpp" +#include "query/v2/request_router.hpp" + +// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) +DEFINE_string(split_file, "", + "Path to the split file which contains the predefined labels, properties, edge types and shard-ranges."); +// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) +DEFINE_string(init_queries_file, "", + "Path to the split file which contains the predefined labels, properties, edge types and shard-ranges."); +// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) +DEFINE_string(benchmark_queries_file, "", + "Path to the split file which contains the predefined labels, properties, edge types and shard-ranges."); + +namespace memgraph::tests::manual { + +using io::LatencyHistogramSummaries; +using query::v2::DiscardValueResultStream; +using query::v2::Interpreter; +using query::v2::InterpreterContext; + +void RunQueries(InterpreterContext &interpreter_context, const std::vector &queries) { + Interpreter interpreter{&interpreter_context}; + DiscardValueResultStream stream; + + for (const auto &query : queries) { + auto result = interpreter.Prepare(query, {}, nullptr); + interpreter.Pull(&stream, std::nullopt, result.qid); + } +} + +void RunInitQueries(InterpreterContext &interpreter_context, const std::vector &init_queries) { + RunQueries(interpreter_context, init_queries); +} + +void RunBenchmarkQueries(InterpreterContext &interpreter_context, const std::vector &benchmark_queries) { + RunQueries(interpreter_context, benchmark_queries); +} + +LatencyHistogramSummaries Run() { + const auto run_start = std::chrono::high_resolution_clock::now(); + std::ifstream sm_file{FLAGS_split_file, std::ios::in}; + MG_ASSERT(sm_file.good(), "Cannot open split file to read: {}", FLAGS_split_file); + auto sm = memgraph::coordinator::ShardMap::Parse(sm_file); + + std::ifstream init_file{FLAGS_init_queries_file, std::ios::in}; + MG_ASSERT(init_file.good(), "Cannot open init queries file to read: {}", FLAGS_init_queries_file); + std::vector init_queries{}; + std::string buffer; + while (init_file.good()) { + std::getline(init_file, buffer); + if (buffer.empty()) { + continue; + } + // Trim the trailing `;` + init_queries.push_back(buffer.substr(0, buffer.size() - 1)); + } + + std::ifstream benchmark_file{FLAGS_benchmark_queries_file, std::ios::in}; + MG_ASSERT(benchmark_file.good(), "Cannot open benchmark queries file to read: {}", FLAGS_benchmark_queries_file); + std::vector benchmark_queries{}; + + while (benchmark_file.good()) { + std::getline(benchmark_file, buffer); + if (buffer.empty()) { + continue; + } + // Trim the trailing `;` + benchmark_queries.push_back(buffer.substr(0, buffer.size() - 1)); + } + + io::local_transport::LocalSystem ls; + + auto unique_local_addr_query = io::Address::UniqueLocalAddress(); + auto io = ls.Register(unique_local_addr_query); + + memgraph::machine_manager::MachineConfig config{ + .coordinator_addresses = std::vector{unique_local_addr_query}, + .is_storage = true, + .is_coordinator = true, + .listen_ip = unique_local_addr_query.last_known_ip, + .listen_port = unique_local_addr_query.last_known_port, + .shard_worker_threads = 2, + }; + + memgraph::coordinator::Coordinator coordinator{sm}; + + memgraph::machine_manager::MachineManager mm{io, config, coordinator}; + std::jthread mm_thread([&mm] { mm.Run(); }); + + auto rr_factory = std::make_unique(io); + + InterpreterContext interpreter_context{(memgraph::storage::v3::Shard *)(nullptr), + {.execution_timeout_sec = 0}, + "data", + std::move(rr_factory), + mm.CoordinatorAddress()}; + + // without this it fails sometimes because the CreateVertices request might reach the shard worker faster than the + // ShardToInitialize + std::this_thread::sleep_for(std::chrono::milliseconds(150)); + + const auto init_start = std::chrono::high_resolution_clock::now(); + RunInitQueries(interpreter_context, init_queries); + const auto benchmark_start = std::chrono::high_resolution_clock::now(); + RunBenchmarkQueries(interpreter_context, benchmark_queries); + const auto benchmark_end = std::chrono::high_resolution_clock::now(); + + spdlog::critical("Read: {}ms", std::chrono::duration_cast(init_start - run_start).count()); + spdlog::critical("Init: {}ms", + std::chrono::duration_cast(benchmark_start - init_start).count()); + spdlog::critical("Benchmark: {}ms", + std::chrono::duration_cast(benchmark_end - benchmark_start).count()); + ls.ShutDown(); + return io.ResponseLatencies(); +} +} // namespace memgraph::tests::manual + +int main(int argc, char **argv) { + spdlog::cfg::load_env_levels(); + gflags::ParseCommandLineFlags(&argc, &argv, true); + memgraph::tests::manual::Run(); + return 0; +} From 1bc93b64f40d3daa364cfa7e53479d3768d49d98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Mon, 13 Feb 2023 23:32:33 +0100 Subject: [PATCH 28/56] Make it possible to compile v2 and v3 interpreter into a single binary --- src/expr/ast/cypher_main_visitor.hpp | 8 ++++---- src/parser/CMakeLists.txt | 2 +- src/parser/opencypher/parser.hpp | 19 ++++++++----------- src/query/CMakeLists.txt | 18 ++++++++++-------- src/query/v2/cypher_query_interpreter.cpp | 8 ++++---- src/query/v2/cypher_query_interpreter.hpp | 8 ++++---- src/query/v2/plan/rewrite/index_lookup.cpp | 4 ++-- src/query/v2/plan/rewrite/index_lookup.hpp | 6 +++--- src/query/v2/plan/variable_start_planner.cpp | 4 ++-- src/query/v2/plan/variable_start_planner.hpp | 6 +++--- 10 files changed, 41 insertions(+), 42 deletions(-) diff --git a/src/expr/ast/cypher_main_visitor.hpp b/src/expr/ast/cypher_main_visitor.hpp index 17d2167b2..bbe5d2d06 100644 --- a/src/expr/ast/cypher_main_visitor.hpp +++ b/src/expr/ast/cypher_main_visitor.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -51,7 +51,7 @@ constexpr char kId[] = "ID"; namespace MG_INJECTED_NAMESPACE_NAME { namespace detail { -using antlropencypher::MemgraphCypher; +using antlropencypher::v2::MemgraphCypher; template std::optional> VisitMemoryLimit(MemgraphCypher::MemoryLimitContext *memory_limit_ctx, @@ -211,13 +211,13 @@ inline std::string_view ToString(const PulsarConfigKey key) { } } // namespace detail -using antlropencypher::MemgraphCypher; +using antlropencypher::v2::MemgraphCypher; struct ParsingContext { bool is_query_cached = false; }; -class CypherMainVisitor : public antlropencypher::MemgraphCypherBaseVisitor { +class CypherMainVisitor : public antlropencypher::v2::MemgraphCypherBaseVisitor { public: explicit CypherMainVisitor(ParsingContext context, AstStorage *storage) : context_(context), storage_(storage) {} diff --git a/src/parser/CMakeLists.txt b/src/parser/CMakeLists.txt index 7575b0529..b7ae43178 100644 --- a/src/parser/CMakeLists.txt +++ b/src/parser/CMakeLists.txt @@ -23,7 +23,7 @@ add_custom_command( COMMAND ${CMAKE_COMMAND} -E make_directory ${opencypher_generated} COMMAND java -jar ${CMAKE_SOURCE_DIR}/libs/antlr-4.10.1-complete.jar - -Dlanguage=Cpp -visitor -package antlropencypher + -Dlanguage=Cpp -visitor -package antlropencypher::v2 -o ${opencypher_generated} ${opencypher_lexer_grammar} ${opencypher_parser_grammar} WORKING_DIRECTORY "${CMAKE_BINARY_DIR}" diff --git a/src/parser/opencypher/parser.hpp b/src/parser/opencypher/parser.hpp index 9a57bc65b..25fdabf4b 100644 --- a/src/parser/opencypher/parser.hpp +++ b/src/parser/opencypher/parser.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -14,10 +14,10 @@ #include #include "antlr4-runtime.h" -#include "utils/exceptions.hpp" #include "parser/opencypher/generated/MemgraphCypher.h" #include "parser/opencypher/generated/MemgraphCypherLexer.h" #include "utils/concepts.hpp" +#include "utils/exceptions.hpp" namespace memgraph::frontend::opencypher { @@ -32,11 +32,9 @@ class SyntaxException : public utils::BasicException { * This thing must me a class since parser.cypher() returns pointer and there is * no way for us to get ownership over the object. */ -enum class ParserOpTag : uint8_t { - CYPHER, EXPRESSION -}; +enum class ParserOpTag : uint8_t { CYPHER, EXPRESSION }; -template +template class Parser { public: /** @@ -46,10 +44,9 @@ class Parser { Parser(const std::string query) : query_(std::move(query)) { parser_.removeErrorListeners(); parser_.addErrorListener(&error_listener_); - if constexpr(Tag == ParserOpTag::CYPHER) { + if constexpr (Tag == ParserOpTag::CYPHER) { tree_ = parser_.cypher(); - } - else { + } else { tree_ = parser_.expression(); } if (parser_.getNumberOfSyntaxErrors()) { @@ -75,11 +72,11 @@ class Parser { FirstMessageErrorListener error_listener_; std::string query_; antlr4::ANTLRInputStream input_{query_}; - antlropencypher::MemgraphCypherLexer lexer_{&input_}; + antlropencypher::v2::MemgraphCypherLexer lexer_{&input_}; antlr4::CommonTokenStream tokens_{&lexer_}; // generate ast - antlropencypher::MemgraphCypher parser_{&tokens_}; + antlropencypher::v2::MemgraphCypher parser_{&tokens_}; antlr4::tree::ParseTree *tree_ = nullptr; }; } // namespace memgraph::frontend::opencypher diff --git a/src/query/CMakeLists.txt b/src/query/CMakeLists.txt index 0303f2fa5..d5545cd95 100644 --- a/src/query/CMakeLists.txt +++ b/src/query/CMakeLists.txt @@ -48,18 +48,20 @@ add_dependencies(mg-query generate_lcp_query) target_include_directories(mg-query PUBLIC ${CMAKE_SOURCE_DIR}/include) target_link_libraries(mg-query dl cppitertools Boost::headers) target_link_libraries(mg-query mg-integrations-pulsar mg-integrations-kafka mg-storage-v2 mg-license mg-utils mg-kvstore mg-memory) + if(NOT "${MG_PYTHON_PATH}" STREQUAL "") set(Python3_ROOT_DIR "${MG_PYTHON_PATH}") endif() + if("${MG_PYTHON_VERSION}" STREQUAL "") find_package(Python3 3.5 REQUIRED COMPONENTS Development) else() find_package(Python3 "${MG_PYTHON_VERSION}" EXACT REQUIRED COMPONENTS Development) endif() + target_link_libraries(mg-query Python3::Python) # Generate Antlr openCypher parser - set(opencypher_frontend ${CMAKE_CURRENT_SOURCE_DIR}/frontend/opencypher) set(opencypher_generated ${opencypher_frontend}/generated) set(opencypher_lexer_grammar ${opencypher_frontend}/grammar/MemgraphCypherLexer.g4) @@ -82,15 +84,15 @@ add_custom_command( OUTPUT ${antlr_opencypher_generated_src} ${antlr_opencypher_generated_include} COMMAND ${CMAKE_COMMAND} -E make_directory ${opencypher_generated} COMMAND - java -jar ${CMAKE_SOURCE_DIR}/libs/antlr-4.10.1-complete.jar - -Dlanguage=Cpp -visitor -package antlropencypher - -o ${opencypher_generated} - ${opencypher_lexer_grammar} ${opencypher_parser_grammar} + java -jar ${CMAKE_SOURCE_DIR}/libs/antlr-4.10.1-complete.jar + -Dlanguage=Cpp -visitor -package antlropencypher + -o ${opencypher_generated} + ${opencypher_lexer_grammar} ${opencypher_parser_grammar} WORKING_DIRECTORY "${CMAKE_BINARY_DIR}" DEPENDS - ${opencypher_lexer_grammar} ${opencypher_parser_grammar} - ${opencypher_frontend}/grammar/CypherLexer.g4 - ${opencypher_frontend}/grammar/Cypher.g4) + ${opencypher_lexer_grammar} ${opencypher_parser_grammar} + ${opencypher_frontend}/grammar/CypherLexer.g4 + ${opencypher_frontend}/grammar/Cypher.g4) add_custom_target(generate_opencypher_parser DEPENDS ${antlr_opencypher_generated_src} ${antlr_opencypher_generated_include}) diff --git a/src/query/v2/cypher_query_interpreter.cpp b/src/query/v2/cypher_query_interpreter.cpp index f3f8e48d7..6ac6b36fa 100644 --- a/src/query/v2/cypher_query_interpreter.cpp +++ b/src/query/v2/cypher_query_interpreter.cpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -14,9 +14,9 @@ #include "query/v2/request_router.hpp" // NOLINTNEXTLINE (cppcoreguidelines-avoid-non-const-global-variables) -DEFINE_HIDDEN_bool(query_cost_planner, true, "Use the cost-estimating query planner."); +DEFINE_HIDDEN_bool(query_v2_cost_planner, true, "Use the cost-estimating query planner."); // NOLINTNEXTLINE (cppcoreguidelines-avoid-non-const-global-variables) -DEFINE_VALIDATED_int32(query_plan_cache_ttl, 60, "Time to live for cached query plans, in seconds.", +DEFINE_VALIDATED_int32(query_v2_plan_cache_ttl, 60, "Time to live for cached query plans, in seconds.", FLAG_IN_RANGE(0, std::numeric_limits::max())); namespace memgraph::query::v2 { @@ -123,7 +123,7 @@ std::unique_ptr MakeLogicalPlan(AstStorage ast_storage, CypherQuery auto vertex_counts = plan::MakeVertexCountCache(request_router); auto symbol_table = expr::MakeSymbolTable(query, predefined_identifiers); auto planning_context = plan::MakePlanningContext(&ast_storage, &symbol_table, query, &vertex_counts); - auto [root, cost] = plan::MakeLogicalPlan(&planning_context, parameters, FLAGS_query_cost_planner); + auto [root, cost] = plan::MakeLogicalPlan(&planning_context, parameters, FLAGS_query_v2_cost_planner); return std::make_unique(std::move(root), cost, std::move(ast_storage), std::move(symbol_table)); } diff --git a/src/query/v2/cypher_query_interpreter.hpp b/src/query/v2/cypher_query_interpreter.hpp index 688e52fed..18505820f 100644 --- a/src/query/v2/cypher_query_interpreter.hpp +++ b/src/query/v2/cypher_query_interpreter.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -22,9 +22,9 @@ #include "utils/timer.hpp" // NOLINTNEXTLINE (cppcoreguidelines-avoid-non-const-global-variables) -DECLARE_bool(query_cost_planner); +DECLARE_bool(query_v2_cost_planner); // NOLINTNEXTLINE (cppcoreguidelines-avoid-non-const-global-variables) -DECLARE_int32(query_plan_cache_ttl); +DECLARE_int32(query_v2_plan_cache_ttl); namespace memgraph::query::v2 { @@ -58,7 +58,7 @@ class CachedPlan { bool IsExpired() const { // NOLINTNEXTLINE (modernize-use-nullptr) - return cache_timer_.Elapsed() > std::chrono::seconds(FLAGS_query_plan_cache_ttl); + return cache_timer_.Elapsed() > std::chrono::seconds(FLAGS_query_v2_plan_cache_ttl); }; private: diff --git a/src/query/v2/plan/rewrite/index_lookup.cpp b/src/query/v2/plan/rewrite/index_lookup.cpp index 795f15fa4..5141e92df 100644 --- a/src/query/v2/plan/rewrite/index_lookup.cpp +++ b/src/query/v2/plan/rewrite/index_lookup.cpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -13,7 +13,7 @@ #include "utils/flag_validation.hpp" -DEFINE_VALIDATED_HIDDEN_int64(query_vertex_count_to_expand_existing, 10, +DEFINE_VALIDATED_HIDDEN_int64(query_v2_vertex_count_to_expand_existing, 10, "Maximum count of indexed vertices which provoke " "indexed lookup and then expand to existing, instead of " "a regular expand. Default is 10, to turn off use -1.", diff --git a/src/query/v2/plan/rewrite/index_lookup.hpp b/src/query/v2/plan/rewrite/index_lookup.hpp index 0b9b9cb97..c8d2b3720 100644 --- a/src/query/v2/plan/rewrite/index_lookup.hpp +++ b/src/query/v2/plan/rewrite/index_lookup.hpp @@ -30,7 +30,7 @@ #include "query/v2/plan/preprocess.hpp" #include "storage/v3/id_types.hpp" -DECLARE_int64(query_vertex_count_to_expand_existing); +DECLARE_int64(query_v2_vertex_count_to_expand_existing); namespace memgraph::query::v2::plan { @@ -100,7 +100,7 @@ class IndexLookupRewriter final : public HierarchicalLogicalOperatorVisitor { return true; } ScanAll dst_scan(expand.input(), expand.common_.node_symbol, expand.view_); - auto indexed_scan = GenScanByIndex(dst_scan, FLAGS_query_vertex_count_to_expand_existing); + auto indexed_scan = GenScanByIndex(dst_scan, FLAGS_query_v2_vertex_count_to_expand_existing); if (indexed_scan) { expand.set_input(std::move(indexed_scan)); expand.common_.existing_node = true; @@ -129,7 +129,7 @@ class IndexLookupRewriter final : public HierarchicalLogicalOperatorVisitor { // unconditionally creating an indexed scan. indexed_scan = GenScanByIndex(dst_scan); } else { - indexed_scan = GenScanByIndex(dst_scan, FLAGS_query_vertex_count_to_expand_existing); + indexed_scan = GenScanByIndex(dst_scan, FLAGS_query_v2_vertex_count_to_expand_existing); } if (indexed_scan) { expand.set_input(std::move(indexed_scan)); diff --git a/src/query/v2/plan/variable_start_planner.cpp b/src/query/v2/plan/variable_start_planner.cpp index b6d15f73d..6fd6e0ad6 100644 --- a/src/query/v2/plan/variable_start_planner.cpp +++ b/src/query/v2/plan/variable_start_planner.cpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -17,7 +17,7 @@ #include "utils/flag_validation.hpp" #include "utils/logging.hpp" -DEFINE_VALIDATED_HIDDEN_uint64(query_max_plans, 1000U, "Maximum number of generated plans for a query.", +DEFINE_VALIDATED_HIDDEN_uint64(query_v2_max_plans, 1000U, "Maximum number of generated plans for a query.", FLAG_IN_RANGE(1, std::numeric_limits::max())); namespace memgraph::query::v2::plan::impl { diff --git a/src/query/v2/plan/variable_start_planner.hpp b/src/query/v2/plan/variable_start_planner.hpp index 27722b6b2..abb7f269f 100644 --- a/src/query/v2/plan/variable_start_planner.hpp +++ b/src/query/v2/plan/variable_start_planner.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -18,7 +18,7 @@ #include "query/v2/plan/rule_based_planner.hpp" -DECLARE_uint64(query_max_plans); +DECLARE_uint64(query_v2_max_plans); namespace memgraph::query::v2::plan { @@ -310,7 +310,7 @@ class VariableStartPlanner { for (const auto &query_part : query_parts) { alternative_query_parts.emplace_back(impl::VaryQueryPartMatching(query_part, symbol_table)); } - return iter::slice(MakeCartesianProduct(std::move(alternative_query_parts)), 0UL, FLAGS_query_max_plans); + return iter::slice(MakeCartesianProduct(std::move(alternative_query_parts)), 0UL, FLAGS_query_v2_max_plans); } public: From fe14a8674c66f6f5b204eac4c379d4056f9a392d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Mon, 13 Feb 2023 23:33:01 +0100 Subject: [PATCH 29/56] Include both v2 and v3 in query performance test --- tests/manual/CMakeLists.txt | 11 ++- tests/manual/query_performance.cpp | 142 ++++++++++++++++++++++++----- 2 files changed, 128 insertions(+), 25 deletions(-) diff --git a/tests/manual/CMakeLists.txt b/tests/manual/CMakeLists.txt index a0c5983fd..800cd0515 100644 --- a/tests/manual/CMakeLists.txt +++ b/tests/manual/CMakeLists.txt @@ -9,6 +9,7 @@ function(add_manual_test test_cpp) get_filename_component(exec_name ${test_cpp} NAME_WE) set(target_name ${test_prefix}${exec_name}) add_executable(${target_name} ${test_cpp} ${ARGN}) + # OUTPUT_NAME sets the real name of a target when it is built and can be # used to help create two targets of the same name even though CMake # requires unique logical target names @@ -21,7 +22,7 @@ target_link_libraries(${test_prefix}antlr_parser antlr_opencypher_parser_lib) add_manual_test(antlr_sigsegv.cpp) target_link_libraries(${test_prefix}antlr_sigsegv gtest gtest_main - antlr_opencypher_parser_lib mg-utils) + antlr_opencypher_parser_lib mg-utils) add_manual_test(antlr_tree_pretty_print.cpp) target_link_libraries(${test_prefix}antlr_tree_pretty_print antlr_opencypher_parser_lib) @@ -37,13 +38,15 @@ target_link_libraries(${test_prefix}query_hash mg-query) add_manual_test(query_planner.cpp interactive/planning.cpp) target_link_libraries(${test_prefix}query_planner mg-query) -if (READLINE_FOUND) + +if(READLINE_FOUND) target_link_libraries(${test_prefix}query_planner readline) endif() add_manual_test(query_execution_dummy.cpp) target_link_libraries(${test_prefix}query_execution_dummy mg-query) -if (READLINE_FOUND) + +if(READLINE_FOUND) target_link_libraries(${test_prefix}query_execution_dummy readline) endif() @@ -63,4 +66,4 @@ add_manual_test(ssl_server.cpp) target_link_libraries(${test_prefix}ssl_server mg-communication) add_manual_test(query_performance.cpp) -target_link_libraries(${test_prefix}query_performance mg-communication mg-utils mg-io mg-io-simulator mg-coordinator mg-query-v2 mg-storage-v3) +target_link_libraries(${test_prefix}query_performance mg-communication mg-utils mg-io mg-io-simulator mg-coordinator mg-query-v2 mg-storage-v3 mg-query mg-storage-v2) diff --git a/tests/manual/query_performance.cpp b/tests/manual/query_performance.cpp index 5d550a788..c5da6bc61 100644 --- a/tests/manual/query_performance.cpp +++ b/tests/manual/query_performance.cpp @@ -17,34 +17,61 @@ #include #include +// v3 includes #include "io/address.hpp" #include "io/local_transport/local_system.hpp" #include "io/message_histogram_collector.hpp" #include "machine_manager/machine_manager.hpp" +#include "query/discard_value_stream.hpp" #include "query/v2/discard_value_stream.hpp" #include "query/v2/interpreter.hpp" #include "query/v2/request_router.hpp" +// v2 includes +#include "query/interpreter.hpp" +#include "storage/v2/storage.hpp" + +// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) +DEFINE_string(index_queries_file, "", + "Path to the file which contains the queries to create indices. Used only for v2."); // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) DEFINE_string(split_file, "", - "Path to the split file which contains the predefined labels, properties, edge types and shard-ranges."); + "Path to the split file which contains the predefined labels, properties, edge types and shard-ranges. " + "Used only for v3."); // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) -DEFINE_string(init_queries_file, "", - "Path to the split file which contains the predefined labels, properties, edge types and shard-ranges."); +DEFINE_string(init_queries_file, "", "Path to the file that is used to insert the initial dataset."); // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) -DEFINE_string(benchmark_queries_file, "", - "Path to the split file which contains the predefined labels, properties, edge types and shard-ranges."); +DEFINE_string(benchmark_queries_file, "", "Path to the file that contains the queries that we want to measure."); +// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) +DEFINE_bool(use_v3, true, "If set to true, then Memgraph v3 will be used, otherwise Memgraph v2 will be used."); namespace memgraph::tests::manual { -using io::LatencyHistogramSummaries; -using query::v2::DiscardValueResultStream; -using query::v2::Interpreter; -using query::v2::InterpreterContext; +template +struct DependantTypes {}; -void RunQueries(InterpreterContext &interpreter_context, const std::vector &queries) { - Interpreter interpreter{&interpreter_context}; - DiscardValueResultStream stream; +template <> +struct DependantTypes { + using Interpreter = query::Interpreter; + using DiscardValueResultStream = query::DiscardValueResultStream; +}; + +template <> +struct DependantTypes { + using Interpreter = query::v2::Interpreter; + using DiscardValueResultStream = query::v2::DiscardValueResultStream; +}; + +template +using Interpreter = typename DependantTypes::Interpreter; + +template +using DiscardValueResultStream = typename DependantTypes::DiscardValueResultStream; + +template +void RunQueries(TInterpreterContext &interpreter_context, const std::vector &queries) { + Interpreter interpreter{&interpreter_context}; + DiscardValueResultStream stream; for (const auto &query : queries) { auto result = interpreter.Prepare(query, {}, nullptr); @@ -52,15 +79,84 @@ void RunQueries(InterpreterContext &interpreter_context, const std::vector &init_queries) { +template +void RunInitQueries(TInterpreterContext &interpreter_context, const std::vector &init_queries) { RunQueries(interpreter_context, init_queries); } -void RunBenchmarkQueries(InterpreterContext &interpreter_context, const std::vector &benchmark_queries) { +template +void RunBenchmarkQueries(TInterpreterContext &interpreter_context, const std::vector &benchmark_queries) { RunQueries(interpreter_context, benchmark_queries); } -LatencyHistogramSummaries Run() { +void RunV2() { + const auto run_start = std::chrono::high_resolution_clock::now(); + + std::vector init_queries{}; + std::string buffer; + + std::ifstream indices_file{FLAGS_index_queries_file, std::ios::in}; + MG_ASSERT(indices_file.good(), "Cannot open index queries file to read: {}", FLAGS_index_queries_file); + while (indices_file.good()) { + std::getline(indices_file, buffer); + if (buffer.empty()) { + continue; + } + // Trim the trailing `;` + init_queries.push_back(buffer.substr(0, buffer.size() - 1)); + } + + std::ifstream init_file{FLAGS_init_queries_file, std::ios::in}; + MG_ASSERT(init_file.good(), "Cannot open init queries file to read: {}", FLAGS_init_queries_file); + while (init_file.good()) { + std::getline(init_file, buffer); + if (buffer.empty()) { + continue; + } + // Trim the trailing `;` + init_queries.push_back(buffer.substr(0, buffer.size() - 1)); + } + + std::ifstream benchmark_file{FLAGS_benchmark_queries_file, std::ios::in}; + MG_ASSERT(benchmark_file.good(), "Cannot open benchmark queries file to read: {}", FLAGS_benchmark_queries_file); + std::vector benchmark_queries{}; + + while (benchmark_file.good()) { + std::getline(benchmark_file, buffer); + if (buffer.empty()) { + continue; + } + // Trim the trailing `;` + benchmark_queries.push_back(buffer.substr(0, buffer.size() - 1)); + } + storage::Storage storage{ + storage::Config{.durability{.snapshot_wal_mode = storage::Config::Durability::SnapshotWalMode::DISABLED}}}; + + memgraph::query::InterpreterContext interpreter_context{ + &storage, + {.query = {.allow_load_csv = false}, + .execution_timeout_sec = 0, + .replication_replica_check_frequency = std::chrono::seconds(0), + .default_kafka_bootstrap_servers = "", + .default_pulsar_service_url = "", + .stream_transaction_conflict_retries = 0, + .stream_transaction_retry_interval = std::chrono::milliseconds(0)}, + "query_performance_data"}; + + const auto init_start = std::chrono::high_resolution_clock::now(); + RunInitQueries(interpreter_context, init_queries); + const auto benchmark_start = std::chrono::high_resolution_clock::now(); + spdlog::critical("Read: {}ms", std::chrono::duration_cast(init_start - run_start).count()); + RunBenchmarkQueries(interpreter_context, benchmark_queries); + const auto benchmark_end = std::chrono::high_resolution_clock::now(); + + spdlog::critical("Init: {}ms", + std::chrono::duration_cast(benchmark_start - init_start).count()); + spdlog::critical("Benchmark: {}ms", + std::chrono::duration_cast(benchmark_end - benchmark_start).count()); +} + +io::LatencyHistogramSummaries RunV3() { const auto run_start = std::chrono::high_resolution_clock::now(); std::ifstream sm_file{FLAGS_split_file, std::ios::in}; MG_ASSERT(sm_file.good(), "Cannot open split file to read: {}", FLAGS_split_file); @@ -113,11 +209,11 @@ LatencyHistogramSummaries Run() { auto rr_factory = std::make_unique(io); - InterpreterContext interpreter_context{(memgraph::storage::v3::Shard *)(nullptr), - {.execution_timeout_sec = 0}, - "data", - std::move(rr_factory), - mm.CoordinatorAddress()}; + query::v2::InterpreterContext interpreter_context{(memgraph::storage::v3::Shard *)(nullptr), + {.execution_timeout_sec = 0}, + "data", + std::move(rr_factory), + mm.CoordinatorAddress()}; // without this it fails sometimes because the CreateVertices request might reach the shard worker faster than the // ShardToInitialize @@ -142,6 +238,10 @@ LatencyHistogramSummaries Run() { int main(int argc, char **argv) { spdlog::cfg::load_env_levels(); gflags::ParseCommandLineFlags(&argc, &argv, true); - memgraph::tests::manual::Run(); + if (FLAGS_use_v3) { + memgraph::tests::manual::RunV3(); + } else { + memgraph::tests::manual::RunV2(); + } return 0; } From 6f730f9a913bc4155111b4041fedf4bbe9c09483 Mon Sep 17 00:00:00 2001 From: Josipmrden Date: Tue, 14 Feb 2023 14:11:01 +0100 Subject: [PATCH 30/56] Do not pull vertices one by one --- src/query/v2/plan/operator.cpp | 15 ++++----------- tests/manual/CMakeLists.txt | 1 + 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index 993d24282..17ad40c20 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -3087,25 +3087,18 @@ class DistributedExpandCursor : public Cursor { MG_ASSERT(direction != EdgeAtom::Direction::BOTH); const auto &edge = frame[self_.common_.edge_symbol].ValueEdge(); static constexpr auto get_dst_vertex = [](const EdgeAccessor &edge, - const EdgeAtom::Direction direction) -> msgs::VertexId { + const EdgeAtom::Direction direction) -> accessors::VertexAccessor { switch (direction) { case EdgeAtom::Direction::IN: - return edge.From().Id(); + return edge.From(); case EdgeAtom::Direction::OUT: - return edge.To().Id(); + return edge.To(); case EdgeAtom::Direction::BOTH: throw std::runtime_error("EdgeDirection Both not implemented"); } }; - msgs::GetPropertiesRequest request; - // to not fetch any properties of the edges - request.vertex_ids.push_back(get_dst_vertex(edge, direction)); - auto result_rows = context.request_router->GetProperties(std::move(request)); - MG_ASSERT(result_rows.size() == 1); - auto &result_row = result_rows.front(); - frame[self_.common_.node_symbol] = - accessors::VertexAccessor(msgs::Vertex{result_row.vertex}, result_row.props, context.request_router); + frame[self_.common_.node_symbol] = get_dst_vertex(edge, direction); } bool InitEdges(Frame &frame, ExecutionContext &context) { diff --git a/tests/manual/CMakeLists.txt b/tests/manual/CMakeLists.txt index 800cd0515..f2726f3ec 100644 --- a/tests/manual/CMakeLists.txt +++ b/tests/manual/CMakeLists.txt @@ -66,4 +66,5 @@ add_manual_test(ssl_server.cpp) target_link_libraries(${test_prefix}ssl_server mg-communication) add_manual_test(query_performance.cpp) +target_sources(${test_prefix}query_performance PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/query_performance_run.cpp) target_link_libraries(${test_prefix}query_performance mg-communication mg-utils mg-io mg-io-simulator mg-coordinator mg-query-v2 mg-storage-v3 mg-query mg-storage-v2) From 3a59bee80c08aafecc410f13ce333a4f8045dbc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Tue, 14 Feb 2023 14:33:41 +0100 Subject: [PATCH 31/56] Make profiling easier --- tests/manual/query_performance.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/manual/query_performance.cpp b/tests/manual/query_performance.cpp index c5da6bc61..5fe6a247c 100644 --- a/tests/manual/query_performance.cpp +++ b/tests/manual/query_performance.cpp @@ -90,6 +90,7 @@ void RunBenchmarkQueries(TInterpreterContext &interpreter_context, const std::ve } void RunV2() { + spdlog::critical("Running V2"); const auto run_start = std::chrono::high_resolution_clock::now(); std::vector init_queries{}; @@ -146,17 +147,18 @@ void RunV2() { const auto init_start = std::chrono::high_resolution_clock::now(); RunInitQueries(interpreter_context, init_queries); const auto benchmark_start = std::chrono::high_resolution_clock::now(); - spdlog::critical("Read: {}ms", std::chrono::duration_cast(init_start - run_start).count()); RunBenchmarkQueries(interpreter_context, benchmark_queries); const auto benchmark_end = std::chrono::high_resolution_clock::now(); + spdlog::critical("Read: {}ms", std::chrono::duration_cast(init_start - run_start).count()); spdlog::critical("Init: {}ms", std::chrono::duration_cast(benchmark_start - init_start).count()); spdlog::critical("Benchmark: {}ms", std::chrono::duration_cast(benchmark_end - benchmark_start).count()); } -io::LatencyHistogramSummaries RunV3() { +void RunV3() { + spdlog::critical("Running V3"); const auto run_start = std::chrono::high_resolution_clock::now(); std::ifstream sm_file{FLAGS_split_file, std::ios::in}; MG_ASSERT(sm_file.good(), "Cannot open split file to read: {}", FLAGS_split_file); @@ -231,11 +233,11 @@ io::LatencyHistogramSummaries RunV3() { spdlog::critical("Benchmark: {}ms", std::chrono::duration_cast(benchmark_end - benchmark_start).count()); ls.ShutDown(); - return io.ResponseLatencies(); } } // namespace memgraph::tests::manual int main(int argc, char **argv) { + spdlog::set_level(spdlog::level::warn); spdlog::cfg::load_env_levels(); gflags::ParseCommandLineFlags(&argc, &argv, true); if (FLAGS_use_v3) { From a17010ed16e241ff0b7566b3e9d1bec2de04e49a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 15 Feb 2023 08:37:45 +0100 Subject: [PATCH 32/56] Supress clang-tidy warnings --- src/query/v2/interpreter.cpp | 1 + src/query/v2/multiframe.cpp | 1 + 2 files changed, 2 insertions(+) diff --git a/src/query/v2/interpreter.cpp b/src/query/v2/interpreter.cpp index d85297909..6a414d5d9 100644 --- a/src/query/v2/interpreter.cpp +++ b/src/query/v2/interpreter.cpp @@ -64,6 +64,7 @@ #include "utils/tsc.hpp" #include "utils/variant_helpers.hpp" +// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) DEFINE_bool(use_multi_frame, false, "Whether to use MultiFrame or not"); namespace EventCounter { diff --git a/src/query/v2/multiframe.cpp b/src/query/v2/multiframe.cpp index 264990bb7..8bbba08bf 100644 --- a/src/query/v2/multiframe.cpp +++ b/src/query/v2/multiframe.cpp @@ -17,6 +17,7 @@ #include "query/v2/bindings/frame.hpp" #include "utils/pmr/vector.hpp" +// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) DEFINE_uint64(default_multi_frame_size, 100, "Default size of MultiFrame"); namespace memgraph::query::v2 { From 3b0d531343a201f1009ccd635f9c252dfb82bce3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 15 Feb 2023 09:07:48 +0100 Subject: [PATCH 33/56] Supress clang-tidy warning --- src/query/v2/multiframe.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/query/v2/multiframe.hpp b/src/query/v2/multiframe.hpp index 396092c2b..2092ec4a2 100644 --- a/src/query/v2/multiframe.hpp +++ b/src/query/v2/multiframe.hpp @@ -17,6 +17,7 @@ #include "query/v2/bindings/frame.hpp" +// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) DECLARE_uint64(default_multi_frame_size); namespace memgraph::query::v2 { From 6fc0b9ff02edaee97fe88dc615c8b11f4af9ca6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 15 Feb 2023 09:32:59 +0100 Subject: [PATCH 34/56] Remove reference to non-existing file --- tests/manual/CMakeLists.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/manual/CMakeLists.txt b/tests/manual/CMakeLists.txt index f2726f3ec..800cd0515 100644 --- a/tests/manual/CMakeLists.txt +++ b/tests/manual/CMakeLists.txt @@ -66,5 +66,4 @@ add_manual_test(ssl_server.cpp) target_link_libraries(${test_prefix}ssl_server mg-communication) add_manual_test(query_performance.cpp) -target_sources(${test_prefix}query_performance PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/query_performance_run.cpp) target_link_libraries(${test_prefix}query_performance mg-communication mg-utils mg-io mg-io-simulator mg-coordinator mg-query-v2 mg-storage-v3 mg-query mg-storage-v2) From 6d4dff7e6e0124eaf6ed2d8369a196dddf928073 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 15 Feb 2023 13:53:48 +0100 Subject: [PATCH 35/56] Suppress clang-tidy warnings --- src/query/v2/plan/rewrite/index_lookup.cpp | 1 + src/query/v2/plan/rewrite/index_lookup.hpp | 1 + src/query/v2/plan/variable_start_planner.cpp | 1 + src/query/v2/plan/variable_start_planner.hpp | 1 + 4 files changed, 4 insertions(+) diff --git a/src/query/v2/plan/rewrite/index_lookup.cpp b/src/query/v2/plan/rewrite/index_lookup.cpp index 5141e92df..b12a26da1 100644 --- a/src/query/v2/plan/rewrite/index_lookup.cpp +++ b/src/query/v2/plan/rewrite/index_lookup.cpp @@ -13,6 +13,7 @@ #include "utils/flag_validation.hpp" +// NOLINTNEXTLINE (cppcoreguidelines-avoid-non-const-global-variables) DEFINE_VALIDATED_HIDDEN_int64(query_v2_vertex_count_to_expand_existing, 10, "Maximum count of indexed vertices which provoke " "indexed lookup and then expand to existing, instead of " diff --git a/src/query/v2/plan/rewrite/index_lookup.hpp b/src/query/v2/plan/rewrite/index_lookup.hpp index c8d2b3720..7f5b601e5 100644 --- a/src/query/v2/plan/rewrite/index_lookup.hpp +++ b/src/query/v2/plan/rewrite/index_lookup.hpp @@ -30,6 +30,7 @@ #include "query/v2/plan/preprocess.hpp" #include "storage/v3/id_types.hpp" +// NOLINTNEXTLINE (cppcoreguidelines-avoid-non-const-global-variables) DECLARE_int64(query_v2_vertex_count_to_expand_existing); namespace memgraph::query::v2::plan { diff --git a/src/query/v2/plan/variable_start_planner.cpp b/src/query/v2/plan/variable_start_planner.cpp index 6fd6e0ad6..ec23be1af 100644 --- a/src/query/v2/plan/variable_start_planner.cpp +++ b/src/query/v2/plan/variable_start_planner.cpp @@ -17,6 +17,7 @@ #include "utils/flag_validation.hpp" #include "utils/logging.hpp" +// NOLINTNEXTLINE (cppcoreguidelines-avoid-non-const-global-variables) DEFINE_VALIDATED_HIDDEN_uint64(query_v2_max_plans, 1000U, "Maximum number of generated plans for a query.", FLAG_IN_RANGE(1, std::numeric_limits::max())); diff --git a/src/query/v2/plan/variable_start_planner.hpp b/src/query/v2/plan/variable_start_planner.hpp index abb7f269f..01d7cef58 100644 --- a/src/query/v2/plan/variable_start_planner.hpp +++ b/src/query/v2/plan/variable_start_planner.hpp @@ -18,6 +18,7 @@ #include "query/v2/plan/rule_based_planner.hpp" +// NOLINTNEXTLINE (cppcoreguidelines-avoid-non-const-global-variables) DECLARE_uint64(query_v2_max_plans); namespace memgraph::query::v2::plan { From a2ce9c43965163829e6a21f7ee5d5c68ec36f63f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 15 Feb 2023 13:59:31 +0100 Subject: [PATCH 36/56] Trigger CI --- src/query/v2/interpreter.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/query/v2/interpreter.cpp b/src/query/v2/interpreter.cpp index 6a414d5d9..5fcd433a1 100644 --- a/src/query/v2/interpreter.cpp +++ b/src/query/v2/interpreter.cpp @@ -68,6 +68,7 @@ DEFINE_bool(use_multi_frame, false, "Whether to use MultiFrame or not"); namespace EventCounter { + extern Event ReadQuery; extern Event WriteQuery; extern Event ReadWriteQuery; @@ -77,6 +78,7 @@ extern const Event LabelPropertyIndexCreated; extern const Event StreamsCreated; extern const Event TriggersCreated; + } // namespace EventCounter namespace memgraph::query::v2 { From f194160d7ce755807c99eca3ad13de652c62a81c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Fri, 17 Feb 2023 17:33:24 +0100 Subject: [PATCH 37/56] Add detailed description about the query_performance binary --- tests/manual/query_performance.cpp | 72 +++++++++++++++++++++++-- tests/mgbench/dataset_creator.py | 20 +++++-- tests/mgbench/dataset_creator_unwind.py | 20 +++++-- 3 files changed, 100 insertions(+), 12 deletions(-) diff --git a/tests/manual/query_performance.cpp b/tests/manual/query_performance.cpp index 5fe6a247c..fe0308f82 100644 --- a/tests/manual/query_performance.cpp +++ b/tests/manual/query_performance.cpp @@ -9,6 +9,65 @@ // by the Apache License, Version 2.0, included in the file // licenses/APL.txt. +// This binary is meant to easily compare the performance of: +// - Memgraph v2 +// - Memgraph v3 +// - Memgraph v3 with MultiFrame +// This binary measures three things which provides a high level and easily understandable metric about the performance +// difference between the different versions: +// 1. Read time: how much time does it take to read the files: +// 2. Init time: how much time does it take to run the init queries, including the index creation. For details please +// check RunV2. +// 3. Benchmark time: how much time does it take to run the benchmark queries. +// To quickly compare performance of the different versions just change the query or queries in the benchmark queries +// file you can see the different by running this executable. This way we don't have keep multiple binaries of Memgraph +// v2 and Memgraph v3 with/without MultiFrame, start Memgraph and connect to it with mgconsole and other hassles. As +// everything is run in this binary, it makes easier to generate perf reports/flamegraphs from the query execution of +// different Memgraph versions compared to using the full blown version of Memgraph. +// +// A few important notes: +// - All the input files are mandated to have an empty line at the end of the file as the reading logic expect that. +// - tests/mgbench/dataset_creator_unwind.py is recommended to generate the dataset because it generates queries with +// UNWIND that makes the import faster in Memgraph v3, thus we can compare the performance on non trivial datasets +// also. To make it possible to use the generated dataset, you have to move the generated index queries into a +// separate file that can be supplied as index queries file for this binary when using Memgraph v2. The reason for +// this is Memgraph v3 cannot handle indices yet, thus it crashes. +// - Check the command line flags and their description defined in this file. +// - Also check out the --default-multi-frame-size command line flag if you want to play with that. +// - The log level is manually set to warning in the main function to avoid the overwhelming log messages from Memgraph +// v3. Apart from ease of use, the huge amount of looging can degrade the actual performance. +// +// Example usage with Memgraph v2: +// ./query_performance +// --index-queries-file indices.cypher +// --init-queries-file dataset.cypher +// --benchmark-queries-file benchmark_queries.txt +// --use-v3=false +// +// Example usage with Memgraph v3 without MultiFrame: +// ./query_performance +// --split-file split_file +// --init-queries-file dataset.cypher +// --benchmark-queries-file benchmark_queries.txt +// --use-v3=true +// --use-mutli-frame=false +// +// Example usage with Memgraph v3 with MultiFrame: +// ./query_performance +// --split-file split_file +// --init-queries-file dataset.cypher +// --benchmark-queries-file benchmark_queries.txt +// --use-v3=true +// --use-mutli-frame=true +// +// The examples are using only the necessary flags, however specifying all of them is not a problem, so if you specify +// --index-queries-file for Memgraph v3, then it will be safely ignored just as --split-file for Memgraph v2. +// +// To generate flamegraph you can use the following command: +// flamegraph --cmd "record -F 997 --call-graph fp -g" --root -o flamegraph.svg -- ./query_performance +// Using the default option (dwarf) for --call-graph when calling perf might result in too long runtine of flamegraph +// because of address resolution. See https://github.com/flamegraph-rs/flamegraph/issues/74. + #include #include #include @@ -33,15 +92,20 @@ // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) DEFINE_string(index_queries_file, "", - "Path to the file which contains the queries to create indices. Used only for v2."); + "Path to the file which contains the queries to create indices. Used only for v2. Must contain an empty " + "line at the end of the file after the queries."); // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) DEFINE_string(split_file, "", "Path to the split file which contains the predefined labels, properties, edge types and shard-ranges. " - "Used only for v3."); + "Used only for v3. Must contain an empty line at the end of the file."); // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) -DEFINE_string(init_queries_file, "", "Path to the file that is used to insert the initial dataset."); +DEFINE_string(init_queries_file, "", + "Path to the file that is used to insert the initial dataset, one query per line. Must contain an empty " + "line at the end of the file after the queries."); // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) -DEFINE_string(benchmark_queries_file, "", "Path to the file that contains the queries that we want to measure."); +DEFINE_string(benchmark_queries_file, "", + "Path to the file that contains the queries that we want to compare, one query per line. Must contain an " + "empty line at the end of the file after the queries."); // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) DEFINE_bool(use_v3, true, "If set to true, then Memgraph v3 will be used, otherwise Memgraph v2 will be used."); diff --git a/tests/mgbench/dataset_creator.py b/tests/mgbench/dataset_creator.py index 9ebeb8cd1..0b73eceed 100644 --- a/tests/mgbench/dataset_creator.py +++ b/tests/mgbench/dataset_creator.py @@ -51,10 +51,22 @@ import helpers def main(): parser = argparse.ArgumentParser() - parser.add_argument("--number_of_identities", type=int, default=10) - parser.add_argument("--number_of_files", type=int, default=10) - parser.add_argument("--percentage_of_permissions", type=float, default=1.0) - parser.add_argument("--filename", default="dataset.cypher") + parser.add_argument( + "--number_of_identities", + type=int, + default=10, + help="Determines how many :Identity nodes will the dataset contain.", + ) + parser.add_argument( + "--number_of_files", type=int, default=10, help="Determines how many :File nodes will the dataset contain." + ) + parser.add_argument( + "--percentage_of_permissions", + type=float, + default=1.0, + help="Determines approximately what percentage of the all possible identity-permission-file connections will be created.", + ) + parser.add_argument("--filename", default="dataset.cypher", help="The name of the output file.") args = parser.parse_args() diff --git a/tests/mgbench/dataset_creator_unwind.py b/tests/mgbench/dataset_creator_unwind.py index 564a4d018..00de1c7bf 100644 --- a/tests/mgbench/dataset_creator_unwind.py +++ b/tests/mgbench/dataset_creator_unwind.py @@ -51,10 +51,22 @@ import helpers def main(): parser = argparse.ArgumentParser() - parser.add_argument("--number_of_identities", type=int, default=10) - parser.add_argument("--number_of_files", type=int, default=10) - parser.add_argument("--percentage_of_permissions", type=float, default=1.0) - parser.add_argument("--filename", default="dataset.cypher") + parser.add_argument( + "--number_of_identities", + type=int, + default=10, + help="Determines how many :Identity nodes will the dataset contain.", + ) + parser.add_argument( + "--number_of_files", type=int, default=10, help="Determines how many :File nodes will the dataset contain." + ) + parser.add_argument( + "--percentage_of_permissions", + type=float, + default=1.0, + help="Determines approximately what percentage of the all possible identity-permission-file connections will be created.", + ) + parser.add_argument("--filename", default="dataset.cypher", help="The name of the output file.") args = parser.parse_args() From c50f2c262130933d0927ff21e8a839f11f6b95ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Mon, 20 Feb 2023 12:35:01 +0100 Subject: [PATCH 38/56] Fix e2e tests --- .../distributed_queries/distributed_expand_one.py | 15 ++++++++++++--- .../distributed_queries/distributed_queries.py | 10 ++++++---- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/tests/e2e/distributed_queries/distributed_expand_one.py b/tests/e2e/distributed_queries/distributed_expand_one.py index 9d3d81074..62a09e1e7 100644 --- a/tests/e2e/distributed_queries/distributed_expand_one.py +++ b/tests/e2e/distributed_queries/distributed_expand_one.py @@ -13,7 +13,12 @@ import sys import pytest -from common import connection, execute_and_fetch_all, has_n_result_row, wait_for_shard_manager_to_initialize +from common import ( + connection, + execute_and_fetch_all, + has_n_result_row, + wait_for_shard_manager_to_initialize, +) def test_sequenced_expand_one(connection): @@ -28,8 +33,12 @@ def test_sequenced_expand_one(connection): results = execute_and_fetch_all(cursor, "MATCH (n)-[:TO]->(m)-[:TO]->(l) RETURN n,m,l") assert len(results) == 1 n, m, l = results[0] - assert n.properties["property"] == 1 - assert m.properties["property"] == 2 + assert ( + len(n.properties) == 0 + ), "we don't return any properties of the node received from expansion and the bolt layer doesn't serialize the primary key of vertices" + assert ( + len(m.properties) == 0 + ), "we don't return any properties of the node received from expansion and the bolt layer doesn't serialize the primary key of vertices" assert l.properties["property"] == 3 diff --git a/tests/e2e/distributed_queries/distributed_queries.py b/tests/e2e/distributed_queries/distributed_queries.py index b4b6324d9..7b98598a3 100644 --- a/tests/e2e/distributed_queries/distributed_queries.py +++ b/tests/e2e/distributed_queries/distributed_queries.py @@ -9,11 +9,13 @@ # by the Apache License, Version 2.0, included in the file # licenses/APL.txt. -import typing -import mgclient import sys -import pytest import time +import typing + +import mgclient +import pytest + from common import * @@ -35,7 +37,7 @@ def test_vertex_creation_and_scanall(connection): assert len(results) == 9 for (n, r, m) in results: n_props = n.properties - assert len(n_props) == 1, "n is not expected to have properties, update the test!" + assert len(n_props) == 0, "n is not expected to have properties, update the test!" assert len(n.labels) == 0, "n is not expected to have labels, update the test!" assert r.type == "TO" From b56b1521a6d6c75b012b8c8772482a4817575c2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Mon, 20 Feb 2023 13:01:33 +0100 Subject: [PATCH 39/56] Extract file reading logic into separate function --- tests/manual/query_performance.cpp | 88 +++++++++--------------------- 1 file changed, 25 insertions(+), 63 deletions(-) diff --git a/tests/manual/query_performance.cpp b/tests/manual/query_performance.cpp index fe0308f82..8de3bf65d 100644 --- a/tests/manual/query_performance.cpp +++ b/tests/manual/query_performance.cpp @@ -50,7 +50,7 @@ // --init-queries-file dataset.cypher // --benchmark-queries-file benchmark_queries.txt // --use-v3=true -// --use-mutli-frame=false +// --use-multi-frame=false // // Example usage with Memgraph v3 with MultiFrame: // ./query_performance @@ -58,7 +58,7 @@ // --init-queries-file dataset.cypher // --benchmark-queries-file benchmark_queries.txt // --use-v3=true -// --use-mutli-frame=true +// --use-multi-frame=true // // The examples are using only the necessary flags, however specifying all of them is not a problem, so if you specify // --index-queries-file for Memgraph v3, then it will be safely ignored just as --split-file for Memgraph v2. @@ -153,47 +153,31 @@ void RunBenchmarkQueries(TInterpreterContext &interpreter_context, const std::ve RunQueries(interpreter_context, benchmark_queries); } +std::vector ReadQueries(const std::string &file_name) { + std::vector queries{}; + std::string buffer; + + std::ifstream file{file_name, std::ios::in}; + MG_ASSERT(file.good(), "Cannot open queries file to read: {}", file_name); + while (file.good()) { + std::getline(file, buffer); + if (buffer.empty()) { + continue; + } + // Trim the trailing `;` + queries.push_back(buffer.substr(0, buffer.size() - 1)); + } + return queries; +} + void RunV2() { spdlog::critical("Running V2"); const auto run_start = std::chrono::high_resolution_clock::now(); - std::vector init_queries{}; - std::string buffer; + const auto index_queries = ReadQueries(FLAGS_index_queries_file); + const auto init_queries = ReadQueries(FLAGS_init_queries_file); + const auto benchmark_queries = ReadQueries(FLAGS_benchmark_queries_file); - std::ifstream indices_file{FLAGS_index_queries_file, std::ios::in}; - MG_ASSERT(indices_file.good(), "Cannot open index queries file to read: {}", FLAGS_index_queries_file); - while (indices_file.good()) { - std::getline(indices_file, buffer); - if (buffer.empty()) { - continue; - } - // Trim the trailing `;` - init_queries.push_back(buffer.substr(0, buffer.size() - 1)); - } - - std::ifstream init_file{FLAGS_init_queries_file, std::ios::in}; - MG_ASSERT(init_file.good(), "Cannot open init queries file to read: {}", FLAGS_init_queries_file); - while (init_file.good()) { - std::getline(init_file, buffer); - if (buffer.empty()) { - continue; - } - // Trim the trailing `;` - init_queries.push_back(buffer.substr(0, buffer.size() - 1)); - } - - std::ifstream benchmark_file{FLAGS_benchmark_queries_file, std::ios::in}; - MG_ASSERT(benchmark_file.good(), "Cannot open benchmark queries file to read: {}", FLAGS_benchmark_queries_file); - std::vector benchmark_queries{}; - - while (benchmark_file.good()) { - std::getline(benchmark_file, buffer); - if (buffer.empty()) { - continue; - } - // Trim the trailing `;` - benchmark_queries.push_back(buffer.substr(0, buffer.size() - 1)); - } storage::Storage storage{ storage::Config{.durability{.snapshot_wal_mode = storage::Config::Durability::SnapshotWalMode::DISABLED}}}; @@ -209,6 +193,7 @@ void RunV2() { "query_performance_data"}; const auto init_start = std::chrono::high_resolution_clock::now(); + RunInitQueries(interpreter_context, index_queries); RunInitQueries(interpreter_context, init_queries); const auto benchmark_start = std::chrono::high_resolution_clock::now(); RunBenchmarkQueries(interpreter_context, benchmark_queries); @@ -228,31 +213,8 @@ void RunV3() { MG_ASSERT(sm_file.good(), "Cannot open split file to read: {}", FLAGS_split_file); auto sm = memgraph::coordinator::ShardMap::Parse(sm_file); - std::ifstream init_file{FLAGS_init_queries_file, std::ios::in}; - MG_ASSERT(init_file.good(), "Cannot open init queries file to read: {}", FLAGS_init_queries_file); - std::vector init_queries{}; - std::string buffer; - while (init_file.good()) { - std::getline(init_file, buffer); - if (buffer.empty()) { - continue; - } - // Trim the trailing `;` - init_queries.push_back(buffer.substr(0, buffer.size() - 1)); - } - - std::ifstream benchmark_file{FLAGS_benchmark_queries_file, std::ios::in}; - MG_ASSERT(benchmark_file.good(), "Cannot open benchmark queries file to read: {}", FLAGS_benchmark_queries_file); - std::vector benchmark_queries{}; - - while (benchmark_file.good()) { - std::getline(benchmark_file, buffer); - if (buffer.empty()) { - continue; - } - // Trim the trailing `;` - benchmark_queries.push_back(buffer.substr(0, buffer.size() - 1)); - } + const auto init_queries = ReadQueries(FLAGS_init_queries_file); + const auto benchmark_queries = ReadQueries(FLAGS_benchmark_queries_file); io::local_transport::LocalSystem ls; From d2156683d3cd6c11e68f2c0a14b8291830d920d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Mon, 20 Feb 2023 13:46:13 +0100 Subject: [PATCH 40/56] Suuport multiple benchmark queries files --- tests/manual/query_performance.cpp | 57 +++++++++++++++++++++++------- 1 file changed, 45 insertions(+), 12 deletions(-) diff --git a/tests/manual/query_performance.cpp b/tests/manual/query_performance.cpp index 8de3bf65d..abc7206f6 100644 --- a/tests/manual/query_performance.cpp +++ b/tests/manual/query_performance.cpp @@ -41,14 +41,14 @@ // ./query_performance // --index-queries-file indices.cypher // --init-queries-file dataset.cypher -// --benchmark-queries-file benchmark_queries.txt +// --benchmark-queries-files expand.cypher,match.cypyher // --use-v3=false // // Example usage with Memgraph v3 without MultiFrame: // ./query_performance // --split-file split_file // --init-queries-file dataset.cypher -// --benchmark-queries-file benchmark_queries.txt +// --benchmark-queries-files expand.cypher,match.cypyher // --use-v3=true // --use-multi-frame=false // @@ -56,7 +56,7 @@ // ./query_performance // --split-file split_file // --init-queries-file dataset.cypher -// --benchmark-queries-file benchmark_queries.txt +// --benchmark-queries-files expand.cypher,match.cypyher // --use-v3=true // --use-multi-frame=true // @@ -69,6 +69,7 @@ // because of address resolution. See https://github.com/flamegraph-rs/flamegraph/issues/74. #include +#include #include #include @@ -90,6 +91,9 @@ #include "query/interpreter.hpp" #include "storage/v2/storage.hpp" +// common includes +#include "utils/string.hpp" + // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) DEFINE_string(index_queries_file, "", "Path to the file which contains the queries to create indices. Used only for v2. Must contain an empty " @@ -103,9 +107,9 @@ DEFINE_string(init_queries_file, "", "Path to the file that is used to insert the initial dataset, one query per line. Must contain an empty " "line at the end of the file after the queries."); // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) -DEFINE_string(benchmark_queries_file, "", - "Path to the file that contains the queries that we want to compare, one query per line. Must contain an " - "empty line at the end of the file after the queries."); +DEFINE_string(benchmark_queries_files, "", + "Comma separated paths to the files that contain the queries that we want to compare, one query per " + "line. Must contain an empty line at the end of each file after the queries."); // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) DEFINE_bool(use_v3, true, "If set to true, then Memgraph v3 will be used, otherwise Memgraph v2 will be used."); @@ -170,13 +174,23 @@ std::vector ReadQueries(const std::string &file_name) { return queries; } +std::map> ReadBenchmarkQueries(const std::string benchmark_queries_files) { + auto benchmark_files = utils::Split(benchmark_queries_files, ","); + std::map> result; + for (const auto &benchmark_file : benchmark_files) { + const auto path = std::filesystem::path(benchmark_file); + result.emplace(path.stem().string(), ReadQueries(benchmark_file)); + } + return result; +} + void RunV2() { spdlog::critical("Running V2"); const auto run_start = std::chrono::high_resolution_clock::now(); const auto index_queries = ReadQueries(FLAGS_index_queries_file); const auto init_queries = ReadQueries(FLAGS_init_queries_file); - const auto benchmark_queries = ReadQueries(FLAGS_benchmark_queries_file); + const auto benchmarks = ReadBenchmarkQueries(FLAGS_benchmark_queries_files); storage::Storage storage{ storage::Config{.durability{.snapshot_wal_mode = storage::Config::Durability::SnapshotWalMode::DISABLED}}}; @@ -196,12 +210,21 @@ void RunV2() { RunInitQueries(interpreter_context, index_queries); RunInitQueries(interpreter_context, init_queries); const auto benchmark_start = std::chrono::high_resolution_clock::now(); - RunBenchmarkQueries(interpreter_context, benchmark_queries); - const auto benchmark_end = std::chrono::high_resolution_clock::now(); spdlog::critical("Read: {}ms", std::chrono::duration_cast(init_start - run_start).count()); spdlog::critical("Init: {}ms", std::chrono::duration_cast(benchmark_start - init_start).count()); + + for (const auto &[name, queries] : benchmarks) { + const auto current_start = std::chrono::high_resolution_clock::now(); + RunBenchmarkQueries(interpreter_context, queries); + const auto current_stop = std::chrono::high_resolution_clock::now(); + + spdlog::critical("Benchmark {}: {}ms", name, + std::chrono::duration_cast(current_stop - current_start).count()); + } + + const auto benchmark_end = std::chrono::high_resolution_clock::now(); spdlog::critical("Benchmark: {}ms", std::chrono::duration_cast(benchmark_end - benchmark_start).count()); } @@ -214,7 +237,7 @@ void RunV3() { auto sm = memgraph::coordinator::ShardMap::Parse(sm_file); const auto init_queries = ReadQueries(FLAGS_init_queries_file); - const auto benchmark_queries = ReadQueries(FLAGS_benchmark_queries_file); + const auto benchmarks = ReadBenchmarkQueries(FLAGS_benchmark_queries_files); io::local_transport::LocalSystem ls; @@ -250,14 +273,24 @@ void RunV3() { const auto init_start = std::chrono::high_resolution_clock::now(); RunInitQueries(interpreter_context, init_queries); const auto benchmark_start = std::chrono::high_resolution_clock::now(); - RunBenchmarkQueries(interpreter_context, benchmark_queries); - const auto benchmark_end = std::chrono::high_resolution_clock::now(); spdlog::critical("Read: {}ms", std::chrono::duration_cast(init_start - run_start).count()); spdlog::critical("Init: {}ms", std::chrono::duration_cast(benchmark_start - init_start).count()); + + for (const auto &[name, queries] : benchmarks) { + const auto current_start = std::chrono::high_resolution_clock::now(); + RunBenchmarkQueries(interpreter_context, queries); + const auto current_stop = std::chrono::high_resolution_clock::now(); + + spdlog::critical("Benchmark {}: {}ms", name, + std::chrono::duration_cast(current_stop - current_start).count()); + } + + const auto benchmark_end = std::chrono::high_resolution_clock::now(); spdlog::critical("Benchmark: {}ms", std::chrono::duration_cast(benchmark_end - benchmark_start).count()); + ls.ShutDown(); } } // namespace memgraph::tests::manual From c7c488bb46763f7ea0ad57ddd4679e11771ee2e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Mon, 20 Feb 2023 14:53:13 +0100 Subject: [PATCH 41/56] Add flag to output results in json --- tests/manual/query_performance.cpp | 45 +++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 4 deletions(-) diff --git a/tests/manual/query_performance.cpp b/tests/manual/query_performance.cpp index abc7206f6..5bbc86125 100644 --- a/tests/manual/query_performance.cpp +++ b/tests/manual/query_performance.cpp @@ -70,12 +70,14 @@ #include #include +#include #include #include #include #include #include +#include // v3 includes #include "io/address.hpp" @@ -113,6 +115,8 @@ DEFINE_string(benchmark_queries_files, "", // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) DEFINE_bool(use_v3, true, "If set to true, then Memgraph v3 will be used, otherwise Memgraph v2 will be used."); +DEFINE_string(export_json_results, "", "If not empty, then the results will be exported as a json file."); + namespace memgraph::tests::manual { template @@ -130,6 +134,11 @@ struct DependantTypes { using DiscardValueResultStream = query::v2::DiscardValueResultStream; }; +template +void PutResult(nlohmann::json &json, const std::string_view name, std::chrono::duration duration) { + json[name] = std::chrono::duration_cast(duration).count(); +} + template using Interpreter = typename DependantTypes::Interpreter; @@ -215,18 +224,32 @@ void RunV2() { spdlog::critical("Init: {}ms", std::chrono::duration_cast(benchmark_start - init_start).count()); + std::map benchmark_results; for (const auto &[name, queries] : benchmarks) { const auto current_start = std::chrono::high_resolution_clock::now(); RunBenchmarkQueries(interpreter_context, queries); const auto current_stop = std::chrono::high_resolution_clock::now(); - + const auto elapsed = current_stop - current_start; spdlog::critical("Benchmark {}: {}ms", name, - std::chrono::duration_cast(current_stop - current_start).count()); + std::chrono::duration_cast(elapsed).count()); + benchmark_results.emplace(name, elapsed); } const auto benchmark_end = std::chrono::high_resolution_clock::now(); spdlog::critical("Benchmark: {}ms", std::chrono::duration_cast(benchmark_end - benchmark_start).count()); + + if (!FLAGS_export_json_results.empty()) { + nlohmann::json results; + PutResult(results, "init", benchmark_start - init_start); + nlohmann::json benchmark_results_json; + for (const auto &[name, duration] : benchmark_results) { + PutResult(benchmark_results_json, name, duration); + } + results["benchmarks"] = std::move(benchmark_results_json); + std::ofstream results_file{FLAGS_export_json_results}; + results_file << results.dump(); + } } void RunV3() { @@ -278,13 +301,15 @@ void RunV3() { spdlog::critical("Init: {}ms", std::chrono::duration_cast(benchmark_start - init_start).count()); + std::map benchmark_results; for (const auto &[name, queries] : benchmarks) { const auto current_start = std::chrono::high_resolution_clock::now(); RunBenchmarkQueries(interpreter_context, queries); const auto current_stop = std::chrono::high_resolution_clock::now(); - + const auto elapsed = current_stop - current_start; spdlog::critical("Benchmark {}: {}ms", name, - std::chrono::duration_cast(current_stop - current_start).count()); + std::chrono::duration_cast(elapsed).count()); + benchmark_results.emplace(name, elapsed); } const auto benchmark_end = std::chrono::high_resolution_clock::now(); @@ -292,6 +317,18 @@ void RunV3() { std::chrono::duration_cast(benchmark_end - benchmark_start).count()); ls.ShutDown(); + + if (!FLAGS_export_json_results.empty()) { + nlohmann::json results; + PutResult(results, "init", benchmark_start - init_start); + nlohmann::json benchmark_results_json; + for (const auto &[name, duration] : benchmark_results) { + PutResult(benchmark_results_json, name, duration); + } + results["benchmarks"] = std::move(benchmark_results_json); + std::ofstream results_file{FLAGS_export_json_results}; + results_file << results.dump(); + } } } // namespace memgraph::tests::manual From 92da012f9ee486e793f87128accd6c3d77b524c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Tue, 21 Feb 2023 14:02:17 +0100 Subject: [PATCH 42/56] Add flag to binary to control data directory --- tests/manual/query_performance.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/manual/query_performance.cpp b/tests/manual/query_performance.cpp index 5bbc86125..b766d5fbb 100644 --- a/tests/manual/query_performance.cpp +++ b/tests/manual/query_performance.cpp @@ -117,6 +117,8 @@ DEFINE_bool(use_v3, true, "If set to true, then Memgraph v3 will be used, otherw DEFINE_string(export_json_results, "", "If not empty, then the results will be exported as a json file."); +DEFINE_string(data_directory, "mg_data", "Path to directory to use as storage directory for Memgraph v2."); + namespace memgraph::tests::manual { template @@ -202,7 +204,8 @@ void RunV2() { const auto benchmarks = ReadBenchmarkQueries(FLAGS_benchmark_queries_files); storage::Storage storage{ - storage::Config{.durability{.snapshot_wal_mode = storage::Config::Durability::SnapshotWalMode::DISABLED}}}; + storage::Config{.durability{.storage_directory = FLAGS_data_directory, + .snapshot_wal_mode = storage::Config::Durability::SnapshotWalMode::DISABLED}}}; memgraph::query::InterpreterContext interpreter_context{ &storage, @@ -213,7 +216,7 @@ void RunV2() { .default_pulsar_service_url = "", .stream_transaction_conflict_retries = 0, .stream_transaction_retry_interval = std::chrono::milliseconds(0)}, - "query_performance_data"}; + FLAGS_data_directory}; const auto init_start = std::chrono::high_resolution_clock::now(); RunInitQueries(interpreter_context, index_queries); From c3042906c56073385c6652209ddccf75554260bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Tue, 21 Feb 2023 14:03:17 +0100 Subject: [PATCH 43/56] Add base of query performance test runner --- tests/manual/query_performance_runner.py | 97 ++++++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 tests/manual/query_performance_runner.py diff --git a/tests/manual/query_performance_runner.py b/tests/manual/query_performance_runner.py new file mode 100644 index 000000000..25444c02c --- /dev/null +++ b/tests/manual/query_performance_runner.py @@ -0,0 +1,97 @@ +# Copyright 2023 Memgraph Ltd. +# +# Use of this software is governed by the Business Source License +# included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source +# License, and you may not use this file except in compliance with the Business Source License. +# +# As of the Change Date specified in that file, in accordance with +# the Business Source License, use of this software will be governed +# by the Apache License, Version 2.0, included in the file +# licenses/APL.txt. + +import argparse +import json +import os +import subprocess +import tempfile + +SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) +PROJECT_DIR = os.path.normpath(os.path.join(SCRIPT_DIR, "..", "..")) +BUILD_DIR = os.path.join(PROJECT_DIR, "build") +BINARY_DIR = os.path.join(BUILD_DIR, "tests/manual") +DEFAULT_BENCHMARK_DIR = os.path.join(BINARY_DIR, "query_performance_benchmark") + +parser = argparse.ArgumentParser(formatter_class=argparse.ArgumentDefaultsHelpFormatter) +parser.add_argument( + "--binary", + type=str, + default=os.path.join(BINARY_DIR, "query_performance"), + help="Path to the binary to use for the benchmark.", +) +parser.add_argument( + "--data-dir", + type=str, + default=tempfile.TemporaryDirectory().name, + help="Path to directory that can be used as a data directory for ", +) +parser.add_argument( + "--summary-path", + type=str, + default=os.path.join(DEFAULT_BENCHMARK_DIR, "summary.json"), + help="Path to which file write the summary.", +) + +parser.add_argument("--init-queries-file", type=str, default=os.path.join(DEFAULT_BENCHMARK_DIR, "dataset.cypher")) +parser.add_argument("--index-queries-file", type=str, default=os.path.join(DEFAULT_BENCHMARK_DIR, "indices.cypher")) +parser.add_argument("--split-file", type=str, default=os.path.join(DEFAULT_BENCHMARK_DIR, "split_file")) + +parser.add_argument( + "--benchmark-queries-files", + type=str, + default=",".join( + [os.path.join(DEFAULT_BENCHMARK_DIR, file_name) for file_name in ["expand.cypher", "match_files.cypher"]] + ), +) + +args = parser.parse_args() + +v2_results_path = os.path.join(DEFAULT_BENCHMARK_DIR, "v2_results.json") +v3_results_path = os.path.join(DEFAULT_BENCHMARK_DIR, "v3_results.json") + + +subprocess.run( + [ + args.binary, + f"--split-file={args.split_file}", + f"--index-queries-file={args.index_queries_file}", + f"--init-queries-file={args.init_queries_file}", + f"--benchmark-queries-files={args.benchmark_queries_files}", + "--use-v3=false", + "--use-multi-frame=true", + f"--export-json-results={v2_results_path}", + f"--data-directory={args.data_dir}", + ] +) + +subprocess.run( + [ + args.binary, + f"--split-file={args.split_file}", + f"--index-queries-file={args.index_queries_file}", + f"--init-queries-file={args.init_queries_file}", + f"--benchmark-queries-files={args.benchmark_queries_files}", + "--use-v3=true", + "--use-multi-frame=true", + f"--export-json-results={v3_results_path}", + f"--data-directory={args.data_dir}", + ] +) + + +v2_results_file = open(v2_results_path) +v2_results = json.load(v2_results_file) +v3_results_file = open(v3_results_path) +v3_results = json.load(v3_results_file) + +with open(args.summary_path, "w") as summary: + json.dump({"v2": v2_results, "v3": v3_results}, summary) From 4cb63e44d9307b65a14197b54d5e760ddebb1549 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Tue, 21 Feb 2023 21:37:00 +0100 Subject: [PATCH 44/56] Download benchmark data --- tests/manual/query_performance_runner.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/manual/query_performance_runner.py b/tests/manual/query_performance_runner.py index 25444c02c..25ae158f3 100644 --- a/tests/manual/query_performance_runner.py +++ b/tests/manual/query_performance_runner.py @@ -10,16 +10,23 @@ # licenses/APL.txt. import argparse +import io import json import os import subprocess +import tarfile import tempfile +import requests + SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) PROJECT_DIR = os.path.normpath(os.path.join(SCRIPT_DIR, "..", "..")) BUILD_DIR = os.path.join(PROJECT_DIR, "build") BINARY_DIR = os.path.join(BUILD_DIR, "tests/manual") DEFAULT_BENCHMARK_DIR = os.path.join(BINARY_DIR, "query_performance_benchmark") +DATA_URL = ( + "https://s3.eu-west-1.amazonaws.com/deps.memgraph.io/dataset/query_performance/query_performance_benchmark.tar.gz" +) parser = argparse.ArgumentParser(formatter_class=argparse.ArgumentDefaultsHelpFormatter) parser.add_argument( @@ -59,6 +66,16 @@ v2_results_path = os.path.join(DEFAULT_BENCHMARK_DIR, "v2_results.json") v3_results_path = os.path.join(DEFAULT_BENCHMARK_DIR, "v3_results.json") +if os.path.exists(DEFAULT_BENCHMARK_DIR): + print(f"Using cachced data from {DEFAULT_BENCHMARK_DIR}") +else: + print(f"Downloading benchmark data to {DEFAULT_BENCHMARK_DIR}") + r = requests.get(DATA_URL) + assert r.ok, "Cannot download data" + file_like_object = io.BytesIO(r.content) + tar = tarfile.open(fileobj=file_like_object) + tar.extractall(os.path.dirname(DEFAULT_BENCHMARK_DIR)) + subprocess.run( [ args.binary, From c3446459788bc34c5214b1ecf7dc29dc95dc803c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Tue, 21 Feb 2023 21:43:05 +0100 Subject: [PATCH 45/56] Add sheebang to runner --- tests/manual/query_performance_runner.py | 2 ++ 1 file changed, 2 insertions(+) mode change 100644 => 100755 tests/manual/query_performance_runner.py diff --git a/tests/manual/query_performance_runner.py b/tests/manual/query_performance_runner.py old mode 100644 new mode 100755 index 25ae158f3..fa8b1b4b3 --- a/tests/manual/query_performance_runner.py +++ b/tests/manual/query_performance_runner.py @@ -1,3 +1,5 @@ +#!/usr/bin/env python3 + # Copyright 2023 Memgraph Ltd. # # Use of this software is governed by the Business Source License From 51ed451b82e5269f1aec133c3f84cb57409cf1a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Tue, 21 Feb 2023 21:46:49 +0100 Subject: [PATCH 46/56] Run nquery performance benchmark on CI/CD --- .github/workflows/diff.yaml | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/.github/workflows/diff.yaml b/.github/workflows/diff.yaml index a7faa7d22..321937956 100644 --- a/.github/workflows/diff.yaml +++ b/.github/workflows/diff.yaml @@ -271,3 +271,30 @@ jobs: source ve3/bin/activate cd e2e LD_LIBRARY_PATH=$LD_LIBRARY_PATH:../../libs/mgclient/lib python runner.py --workloads-root-directory ./distributed_queries + + - name: Run query performance tests + run: | + cd tests/manual + ./query_performance_runner.py + + - name: Get branch name (merge) + if: github.event_name != 'pull_request' + shell: bash + run: echo "BRANCH_NAME=$(echo ${GITHUB_REF#refs/heads/} | tr / -)" >> $GITHUB_ENV + + - name: Get branch name (pull request) + if: github.event_name == 'pull_request' + shell: bash + run: echo "BRANCH_NAME=$(echo ${GITHUB_HEAD_REF} | tr / -)" >> $GITHUB_ENV + + - name: Upload macro benchmark results + run: | + cd tools/bench-graph-client + virtualenv -p python3 ve3 + source ve3/bin/activate + pip install -r requirements.txt + ./main.py --benchmark-name "query_performance" \ + --benchmark-results-path "../../build/tests/manual/query_performance_benchmark/summary.json" \ + --github-run-id "${{ github.run_id }}" \ + --github-run-number "${{ github.run_number }}" \ + --head-branch-name "${{ env.BRANCH_NAME }}" From 24d2353e7a216cac7d305a04f6060167d049e41b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Fri, 24 Feb 2023 08:56:13 +0100 Subject: [PATCH 47/56] Move primary keys --- src/storage/v3/request_helper.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/storage/v3/request_helper.cpp b/src/storage/v3/request_helper.cpp index f13c5a82e..f5d72ed15 100644 --- a/src/storage/v3/request_helper.cpp +++ b/src/storage/v3/request_helper.cpp @@ -46,7 +46,6 @@ struct VertexIdCmpr { std::optional> PrimaryKeysFromAccessor(const VertexAccessor &acc, View view, const Schemas::Schema &schema) { std::map ret; - auto props = acc.Properties(view); auto maybe_pk = acc.PrimaryKey(view); if (maybe_pk.HasError()) { spdlog::debug("Encountered an error while trying to get vertex primary key."); @@ -58,7 +57,7 @@ std::optional> PrimaryKeysFromAccessor(const VertexA ret.emplace(schema.second[i].property_id, FromPropertyValueToValue(std::move(pk[i]))); } - return ret; + return {std::move(ret)}; } ShardResult> FillUpSourceVertexSecondaryLabels(const std::optional &v_acc, @@ -99,7 +98,7 @@ ShardResult> FillUpSourceVertexProperties(const std: } auto pks = PrimaryKeysFromAccessor(*v_acc, view, schema); if (pks) { - src_vertex_properties.merge(*pks); + src_vertex_properties.merge(std::move(*pks)); } } else if (req.src_vertex_properties.value().empty()) { From f414b13905a10136eacd17f916da9cac4a1f043e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Fri, 24 Feb 2023 11:32:48 +0100 Subject: [PATCH 48/56] Move requests in transport layer --- src/io/future.hpp | 6 +-- src/io/local_transport/local_transport.hpp | 8 ++-- .../local_transport_handle.hpp | 4 +- src/io/rsm/raft.hpp | 38 +++++++++--------- src/io/rsm/rsm_client.hpp | 27 ++++++------- src/io/simulator/simulator_handle.hpp | 8 ++-- src/io/simulator/simulator_transport.hpp | 10 ++--- src/io/transport.hpp | 40 +++++++++---------- src/query/v2/request_router.hpp | 28 ++++++------- src/storage/v3/shard_manager.hpp | 7 ++-- src/storage/v3/shard_rsm.hpp | 4 +- src/storage/v3/shard_worker.hpp | 4 +- tests/simulation/basic_request.cpp | 4 +- .../query_storage_test.cpp | 4 +- tests/unit/local_transport.cpp | 4 +- 15 files changed, 97 insertions(+), 99 deletions(-) diff --git a/src/io/future.hpp b/src/io/future.hpp index 585f18938..99906f2de 100644 --- a/src/io/future.hpp +++ b/src/io/future.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -120,7 +120,7 @@ class Shared { MG_ASSERT(!consumed_, "Promise filled after it was already consumed!"); MG_ASSERT(!filled_, "Promise filled twice!"); - item_ = item; + item_ = std::move(item); filled_ = true; } // lock released before condition variable notification @@ -235,7 +235,7 @@ class Promise { // Fill the expected item into the Future. void Fill(T item) { MG_ASSERT(!filled_or_moved_, "Promise::Fill called on a promise that is already filled or moved!"); - shared_->Fill(item); + shared_->Fill(std::move(item)); filled_or_moved_ = true; } diff --git a/src/io/local_transport/local_transport.hpp b/src/io/local_transport/local_transport.hpp index b64cabf1d..a071a7dfa 100644 --- a/src/io/local_transport/local_transport.hpp +++ b/src/io/local_transport/local_transport.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -30,10 +30,10 @@ class LocalTransport { explicit LocalTransport(std::shared_ptr local_transport_handle) : local_transport_handle_(std::move(local_transport_handle)) {} - template + template ResponseFuture Request(Address to_address, Address from_address, RequestT request, std::function fill_notifier, Duration timeout) { - return local_transport_handle_->template SubmitRequest( + return local_transport_handle_->template SubmitRequest( to_address, from_address, std::move(request), timeout, fill_notifier); } @@ -44,7 +44,7 @@ class LocalTransport { template void Send(Address to_address, Address from_address, RequestId request_id, M &&message) { - return local_transport_handle_->template Send(to_address, from_address, request_id, std::forward(message)); + return local_transport_handle_->template Send(to_address, from_address, request_id, std::forward(message)); } Time Now() const { return local_transport_handle_->Now(); } diff --git a/src/io/local_transport/local_transport_handle.hpp b/src/io/local_transport/local_transport_handle.hpp index 38538620f..687ab270f 100644 --- a/src/io/local_transport/local_transport_handle.hpp +++ b/src/io/local_transport/local_transport_handle.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -138,7 +138,7 @@ class LocalTransportHandle { cv_.notify_all(); } - template + template ResponseFuture SubmitRequest(Address to_address, Address from_address, RequestT &&request, Duration timeout, std::function fill_notifier) { auto [future, promise] = memgraph::io::FuturePromisePairWithNotifications>( diff --git a/src/io/rsm/raft.hpp b/src/io/rsm/raft.hpp index 07a51288d..0221eedd0 100644 --- a/src/io/rsm/raft.hpp +++ b/src/io/rsm/raft.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -246,7 +246,7 @@ to a CAS operation. template concept Rsm = requires(ReplicatedState state, WriteOperation w, ReadOperation r) { - { state.Read(r) } -> std::same_as; + { state.Read(std::move(r)) } -> std::same_as; { state.Apply(w) } -> std::same_as; }; @@ -402,7 +402,7 @@ class Raft { const PendingClientRequest client_request = std::move(leader.pending_client_requests.at(apply_index)); leader.pending_client_requests.erase(apply_index); - const WriteResponse resp{ + WriteResponse resp{ .success = true, .write_return = std::move(write_return), .raft_index = apply_index, @@ -554,7 +554,7 @@ class Raft { for (const auto &peer : peers_) { // request_id not necessary to set because it's not a Future-backed Request. static constexpr auto request_id = 0; - io_.template Send(peer, request_id, request); + io_.template Send(peer, request_id, request); outstanding_votes.insert(peer); } @@ -624,7 +624,7 @@ class Raft { MG_ASSERT(std::max(req.term, state_.term) == req.term); } - const VoteResponse res{ + VoteResponse res{ .term = std::max(req.term, state_.term), .committed_log_size = state_.committed_log_size, .vote_granted = new_leader, @@ -736,7 +736,7 @@ class Raft { // become follower of this leader, reply with our log status state_.term = req.term; - io_.Send(from_address, request_id, res); + io_.Send(from_address, request_id, AppendResponse{res}); Log("becoming Follower of Leader ", from_address.last_known_port, " at term ", req.term); return Follower{ @@ -747,7 +747,7 @@ class Raft { if (req.term < state_.term) { // nack this request from an old leader - io_.Send(from_address, request_id, res); + io_.Send(from_address, request_id, AppendResponse{res}); return std::nullopt; } @@ -859,17 +859,17 @@ class Raft { auto type_info = TypeInfoFor(req); std::string demangled_name = boost::core::demangle(type_info.get().name()); Log("handling ReadOperation<" + demangled_name + ">"); - ReadOperation read_operation = req.operation; + ReadOperation &read_operation = req.operation; - ReadResponseValue read_return = replicated_state_.Read(read_operation); + ReadResponseValue read_return = replicated_state_.Read(std::move(read_operation)); - const ReadResponse resp{ + ReadResponse resp{ .success = true, .read_return = std::move(read_return), .retry_leader = std::nullopt, }; - io_.Send(from_address, request_id, resp); + io_.Send(from_address, request_id, std::move(resp)); return std::nullopt; } @@ -878,11 +878,11 @@ class Raft { std::optional Handle(Candidate & /* variable */, ReadRequest && /* variable */, RequestId request_id, Address from_address) { Log("received ReadOperation - not redirecting because no Leader is known"); - const ReadResponse res{ + ReadResponse res{ .success = false, }; - io_.Send(from_address, request_id, res); + io_.Send(from_address, request_id, std::move(res)); Cron(); @@ -894,12 +894,12 @@ class Raft { Address from_address) { Log("redirecting client to known Leader with port ", follower.leader_address.last_known_port); - const ReadResponse res{ + ReadResponse res{ .success = false, .retry_leader = follower.leader_address, }; - io_.Send(from_address, request_id, res); + io_.Send(from_address, request_id, std::move(res)); return std::nullopt; } @@ -913,12 +913,12 @@ class Raft { Address from_address) { Log("redirecting client to known Leader with port ", follower.leader_address.last_known_port); - const WriteResponse res{ + WriteResponse res{ .success = false, .retry_leader = follower.leader_address, }; - io_.Send(from_address, request_id, res); + io_.Send(from_address, request_id, std::move(res)); return std::nullopt; } @@ -927,11 +927,11 @@ class Raft { RequestId request_id, Address from_address) { Log("received WriteRequest - not redirecting because no Leader is known"); - const WriteResponse res{ + WriteResponse res{ .success = false, }; - io_.Send(from_address, request_id, res); + io_.Send(from_address, request_id, std::move(res)); Cron(); diff --git a/src/io/rsm/rsm_client.hpp b/src/io/rsm/rsm_client.hpp index 2b76b6399..a98a1dcd2 100644 --- a/src/io/rsm/rsm_client.hpp +++ b/src/io/rsm/rsm_client.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -95,7 +95,7 @@ class RsmClient { BasicResult SendWriteRequest(WriteRequestT req) { Notifier notifier; const ReadinessToken readiness_token{0}; - SendAsyncWriteRequest(req, notifier, readiness_token); + SendAsyncWriteRequest(std::move(req), notifier, readiness_token); auto poll_result = AwaitAsyncWriteRequest(readiness_token); while (!poll_result) { poll_result = AwaitAsyncWriteRequest(readiness_token); @@ -106,7 +106,7 @@ class RsmClient { BasicResult SendReadRequest(ReadRequestT req) { Notifier notifier; const ReadinessToken readiness_token{0}; - SendAsyncReadRequest(req, notifier, readiness_token); + SendAsyncReadRequest(std::move(req), notifier, readiness_token); auto poll_result = AwaitAsyncReadRequest(readiness_token); while (!poll_result) { poll_result = AwaitAsyncReadRequest(readiness_token); @@ -115,15 +115,15 @@ class RsmClient { } /// AsyncRead methods - void SendAsyncReadRequest(const ReadRequestT &req, Notifier notifier, ReadinessToken readiness_token) { + void SendAsyncReadRequest(ReadRequestT &&req, Notifier notifier, ReadinessToken readiness_token) { ReadRequest read_req = {.operation = req}; AsyncRequest> async_request{ .start_time = io_.Now(), .request = std::move(req), .notifier = notifier, - .future = io_.template RequestWithNotification, ReadResponse>( - leader_, read_req, notifier, readiness_token), + .future = io_.template RequestWithNotification>(leader_, std::move(read_req), + notifier, readiness_token), }; async_reads_.emplace(readiness_token.GetId(), std::move(async_request)); @@ -134,8 +134,8 @@ class RsmClient { ReadRequest read_req = {.operation = async_request.request}; - async_request.future = io_.template RequestWithNotification, ReadResponse>( - leader_, read_req, async_request.notifier, readiness_token); + async_request.future = io_.template RequestWithNotification>( + leader_, std::move(read_req), async_request.notifier, readiness_token); } std::optional> PollAsyncReadRequest(const ReadinessToken &readiness_token) { @@ -184,15 +184,15 @@ class RsmClient { } /// AsyncWrite methods - void SendAsyncWriteRequest(const WriteRequestT &req, Notifier notifier, ReadinessToken readiness_token) { + void SendAsyncWriteRequest(WriteRequestT &&req, Notifier notifier, ReadinessToken readiness_token) { WriteRequest write_req = {.operation = req}; AsyncRequest> async_request{ .start_time = io_.Now(), .request = std::move(req), .notifier = notifier, - .future = io_.template RequestWithNotification, WriteResponse>( - leader_, write_req, notifier, readiness_token), + .future = io_.template RequestWithNotification>(leader_, std::move(write_req), + notifier, readiness_token), }; async_writes_.emplace(readiness_token.GetId(), std::move(async_request)); @@ -203,9 +203,8 @@ class RsmClient { WriteRequest write_req = {.operation = async_request.request}; - async_request.future = - io_.template RequestWithNotification, WriteResponse>( - leader_, write_req, async_request.notifier, readiness_token); + async_request.future = io_.template RequestWithNotification>( + leader_, std::move(write_req), async_request.notifier, readiness_token); } std::optional> PollAsyncWriteRequest(const ReadinessToken &readiness_token) { diff --git a/src/io/simulator/simulator_handle.hpp b/src/io/simulator/simulator_handle.hpp index 3fd9b4965..40d28d8c9 100644 --- a/src/io/simulator/simulator_handle.hpp +++ b/src/io/simulator/simulator_handle.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -105,7 +105,7 @@ class SimulatorHandle { bool ShouldShutDown() const; - template + template ResponseFuture SubmitRequest(Address to_address, Address from_address, Request &&request, Duration timeout, std::function &&maybe_tick_simulator, std::function &&fill_notifier) { @@ -194,12 +194,12 @@ class SimulatorHandle { } template - void Send(Address to_address, Address from_address, RequestId request_id, M message) { + void Send(Address to_address, Address from_address, RequestId request_id, M &&message) { spdlog::trace("sending message from {} to {}", from_address.last_known_port, to_address.last_known_port); auto type_info = TypeInfoFor(message); { std::unique_lock lock(mu_); - std::any message_any(std::move(message)); + std::any message_any(std::forward(message)); OpaqueMessage om{.to_address = to_address, .from_address = from_address, .request_id = request_id, diff --git a/src/io/simulator/simulator_transport.hpp b/src/io/simulator/simulator_transport.hpp index 1272a04a1..cc7b6f9b0 100644 --- a/src/io/simulator/simulator_transport.hpp +++ b/src/io/simulator/simulator_transport.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -33,15 +33,15 @@ class SimulatorTransport { SimulatorTransport(std::shared_ptr simulator_handle, Address address, uint64_t seed) : simulator_handle_(simulator_handle), address_(address), rng_(std::mt19937{seed}) {} - template + template ResponseFuture Request(Address to_address, Address from_address, RequestT request, std::function notification, Duration timeout) { std::function tick_simulator = [handle_copy = simulator_handle_] { return handle_copy->MaybeTickSimulator(); }; - return simulator_handle_->template SubmitRequest( - to_address, from_address, std::move(request), timeout, std::move(tick_simulator), std::move(notification)); + return simulator_handle_->template SubmitRequest(to_address, from_address, std::move(request), timeout, + std::move(tick_simulator), std::move(notification)); } template @@ -51,7 +51,7 @@ class SimulatorTransport { template void Send(Address to_address, Address from_address, uint64_t request_id, M message) { - return simulator_handle_->template Send(to_address, from_address, request_id, message); + return simulator_handle_->template Send(to_address, from_address, request_id, message); } Time Now() const { return simulator_handle_->Now(); } diff --git a/src/io/transport.hpp b/src/io/transport.hpp index 5dd7a9a39..bca6250a8 100644 --- a/src/io/transport.hpp +++ b/src/io/transport.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -32,7 +32,7 @@ using memgraph::utils::BasicResult; // reasonable constraints around message types over time, // as we adapt things to use Thrift-generated message types. template -concept Message = std::same_as>; +concept Message = true; using RequestId = uint64_t; @@ -82,44 +82,44 @@ class Io { Duration GetDefaultTimeout() { return default_timeout_; } /// Issue a request with an explicit timeout in microseconds provided. This tends to be used by clients. - template - ResponseFuture RequestWithTimeout(Address address, RequestT request, Duration timeout) { + template + ResponseFuture RequestWithTimeout(Address address, RequestT &&request, Duration timeout) { const Address from_address = address_; std::function fill_notifier = nullptr; - return implementation_.template Request(address, from_address, request, fill_notifier, - timeout); + return implementation_.template Request(address, from_address, std::forward(request), + fill_notifier, timeout); } /// Issue a request that times out after the default timeout. This tends /// to be used by clients. - template - ResponseFuture Request(Address to_address, RequestT request) { + template + ResponseFuture Request(Address to_address, RequestT &&request) { const Duration timeout = default_timeout_; const Address from_address = address_; std::function fill_notifier = nullptr; - return implementation_.template Request(to_address, from_address, std::move(request), - fill_notifier, timeout); + return implementation_.template Request(to_address, from_address, std::forward(request), + fill_notifier, timeout); } /// Issue a request that will notify a Notifier when it is filled or times out. - template - ResponseFuture RequestWithNotification(Address to_address, RequestT request, Notifier notifier, + template + ResponseFuture RequestWithNotification(Address to_address, RequestT &&request, Notifier notifier, ReadinessToken readiness_token) { const Duration timeout = default_timeout_; const Address from_address = address_; std::function fill_notifier = [notifier, readiness_token]() { notifier.Notify(readiness_token); }; - return implementation_.template Request(to_address, from_address, std::move(request), - fill_notifier, timeout); + return implementation_.template Request(to_address, from_address, std::forward(request), + fill_notifier, timeout); } /// Issue a request that will notify a Notifier when it is filled or times out. - template - ResponseFuture RequestWithNotificationAndTimeout(Address to_address, RequestT request, Notifier notifier, + template + ResponseFuture RequestWithNotificationAndTimeout(Address to_address, RequestT &&request, Notifier notifier, ReadinessToken readiness_token, Duration timeout) { const Address from_address = address_; std::function fill_notifier = [notifier, readiness_token]() { notifier.Notify(readiness_token); }; - return implementation_.template Request(to_address, from_address, std::move(request), - fill_notifier, timeout); + return implementation_.template Request(to_address, from_address, std::forward(request), + fill_notifier, timeout); } /// Wait for an explicit number of microseconds for a request of one of the @@ -141,9 +141,9 @@ class Io { /// responses are not necessarily expected, and for servers to respond to requests. /// If you need reliable delivery, this must be built on-top. TCP is not enough for most use cases. template - void Send(Address to_address, RequestId request_id, M message) { + void Send(Address to_address, RequestId request_id, M &&message) { Address from_address = address_; - return implementation_.template Send(to_address, from_address, request_id, std::move(message)); + return implementation_.template Send(to_address, from_address, request_id, std::forward(message)); } /// The current system time. This time source should be preferred over any other, diff --git a/src/query/v2/request_router.hpp b/src/query/v2/request_router.hpp index a8326d900..290bd2cca 100644 --- a/src/query/v2/request_router.hpp +++ b/src/query/v2/request_router.hpp @@ -323,8 +323,8 @@ class RequestRouter : public RequestRouterInterface { io::ReadinessToken readiness_token{i}; auto &storage_client = GetStorageClientForShard(request.shard); msgs::WriteRequests req = request.request; - storage_client.SendAsyncWriteRequest(req, notifier_, readiness_token); - running_requests.emplace(readiness_token.GetId(), request); + storage_client.SendAsyncWriteRequest(std::move(req), notifier_, readiness_token); + running_requests.emplace(readiness_token.GetId(), std::move(request)); } // drive requests to completion @@ -339,7 +339,8 @@ class RequestRouter : public RequestRouterInterface { // must be fetched again with an ExpandOne(Edges.dst) // create requests - std::vector> requests_to_be_sent = RequestsForExpandOne(request); + std::vector> requests_to_be_sent = + RequestsForExpandOne(std::move(request)); // begin all requests in parallel RunningRequests running_requests = {}; @@ -349,8 +350,8 @@ class RequestRouter : public RequestRouterInterface { io::ReadinessToken readiness_token{i}; auto &storage_client = GetStorageClientForShard(request.shard); msgs::ReadRequests req = request.request; - storage_client.SendAsyncReadRequest(req, notifier_, readiness_token); - running_requests.emplace(readiness_token.GetId(), request); + storage_client.SendAsyncReadRequest(std::move(req), notifier_, readiness_token); + running_requests.emplace(readiness_token.GetId(), std::move(request)); } // drive requests to completion @@ -386,8 +387,8 @@ class RequestRouter : public RequestRouterInterface { io::ReadinessToken readiness_token{i}; auto &storage_client = GetStorageClientForShard(request.shard); msgs::ReadRequests req = request.request; - storage_client.SendAsyncReadRequest(req, notifier_, readiness_token); - running_requests.emplace(readiness_token.GetId(), request); + storage_client.SendAsyncReadRequest(std::move(req), notifier_, readiness_token); + running_requests.emplace(readiness_token.GetId(), std::move(request)); } // drive requests to completion @@ -517,7 +518,7 @@ class RequestRouter : public RequestRouterInterface { return requests; } - std::vector> RequestsForExpandOne(const msgs::ExpandOneRequest &request) { + std::vector> RequestsForExpandOne(msgs::ExpandOneRequest &&request) { std::map per_shard_request_table; msgs::ExpandOneRequest top_level_rqst_template = request; top_level_rqst_template.transaction_id = transaction_id_; @@ -529,7 +530,7 @@ class RequestRouter : public RequestRouterInterface { if (!per_shard_request_table.contains(shard)) { per_shard_request_table.insert(std::pair(shard, top_level_rqst_template)); } - per_shard_request_table[shard].src_vertices.push_back(vertex); + per_shard_request_table[shard].src_vertices.push_back(std::move(vertex)); } std::vector> requests = {}; @@ -726,11 +727,10 @@ class RequestRouter : public RequestRouterInterface { coordinator::CoordinatorWriteRequests requests{coordinator::AllocateEdgeIdBatchRequest{.batch_size = 1000000}}; io::rsm::WriteRequest ww; - ww.operation = requests; - auto resp = - io_.template Request, - io::rsm::WriteResponse>(coordinator_address, ww) - .Wait(); + ww.operation = std::move(requests); + auto resp = io_.template Request>( + coordinator_address, std::move(ww)) + .Wait(); if (resp.HasValue()) { const auto alloc_edge_id_reps = std::get(resp.GetValue().message.write_return); diff --git a/src/storage/v3/shard_manager.hpp b/src/storage/v3/shard_manager.hpp index 74ec8d2d1..03cdf7159 100644 --- a/src/storage/v3/shard_manager.hpp +++ b/src/storage/v3/shard_manager.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -249,9 +249,8 @@ class ShardManager { ww.operation = cwr; spdlog::info("SM sending heartbeat to coordinator {}", coordinator_leader_.ToString()); - heartbeat_res_.emplace(std::move( - io_.template Request, WriteResponse>( - coordinator_leader_, ww))); + heartbeat_res_.emplace( + std::move(io_.template Request>(coordinator_leader_, std::move(ww)))); spdlog::info("SM sent heartbeat"); } diff --git a/src/storage/v3/shard_rsm.hpp b/src/storage/v3/shard_rsm.hpp index d301bf40b..18199d8a1 100644 --- a/src/storage/v3/shard_rsm.hpp +++ b/src/storage/v3/shard_rsm.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -42,7 +42,7 @@ class ShardRsm { explicit ShardRsm(std::unique_ptr &&shard) : shard_(std::move(shard)){}; // NOLINTNEXTLINE(readability-convert-member-functions-to-static) - msgs::ReadResponses Read(msgs::ReadRequests requests) { + msgs::ReadResponses Read(msgs::ReadRequests &&requests) { return std::visit([&](auto &&request) mutable { return HandleRead(std::forward(request)); }, std::move(requests)); } diff --git a/src/storage/v3/shard_worker.hpp b/src/storage/v3/shard_worker.hpp index 547aa0a6f..121c4be70 100644 --- a/src/storage/v3/shard_worker.hpp +++ b/src/storage/v3/shard_worker.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -100,7 +100,7 @@ class Queue { inner_->submitted++; - inner_->queue.emplace_back(std::forward(message)); + inner_->queue.emplace_back(std::move(message)); } // lock dropped before notifying condition variable inner_->cv.notify_all(); diff --git a/tests/simulation/basic_request.cpp b/tests/simulation/basic_request.cpp index 868f4ac10..243a57b52 100644 --- a/tests/simulation/basic_request.cpp +++ b/tests/simulation/basic_request.cpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -76,7 +76,7 @@ std::pair RunWorkload(SimulatorConfig CounterRequest cli_req; cli_req.proposal = i; spdlog::info("[CLIENT] calling Request"); - auto res_f = cli_io.Request(srv_addr, cli_req); + auto res_f = cli_io.Request(srv_addr, cli_req); spdlog::info("[CLIENT] calling Wait"); auto res_rez = std::move(res_f).Wait(); spdlog::info("[CLIENT] Wait returned"); diff --git a/tests/simulation/trial_query_storage/query_storage_test.cpp b/tests/simulation/trial_query_storage/query_storage_test.cpp index 8ef12bdb8..eb4f86f68 100644 --- a/tests/simulation/trial_query_storage/query_storage_test.cpp +++ b/tests/simulation/trial_query_storage/query_storage_test.cpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -78,7 +78,7 @@ int main() { auto req = ScanVerticesRequest{2, std::nullopt}; - auto res_f = cli_io.Request(srv_addr, req); + auto res_f = cli_io.Request(srv_addr, req); auto res_rez = std::move(res_f).Wait(); simulator.ShutDown(); return 0; diff --git a/tests/unit/local_transport.cpp b/tests/unit/local_transport.cpp index aa03325de..12ddcd753 100644 --- a/tests/unit/local_transport.cpp +++ b/tests/unit/local_transport.cpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -70,7 +70,7 @@ TEST(LocalTransport, BasicRequest) { auto value = 1; // i; cli_req.proposal = value; spdlog::info("[CLIENT] sending request"); - auto res_f = cli_io.Request(srv_addr, cli_req); + auto res_f = cli_io.Request(srv_addr, cli_req); spdlog::info("[CLIENT] waiting on future"); auto res_rez = std::move(res_f).Wait(); From 86aa488e79b2f42bbbe6d312bd510e2f6f30956a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 1 Mar 2023 08:02:37 +0100 Subject: [PATCH 49/56] Prefer move in transport layer over forward --- src/io/local_transport/local_transport.hpp | 4 ++-- src/io/local_transport/local_transport_handle.hpp | 4 ++-- src/io/rsm/raft.hpp | 6 +++--- src/io/simulator/simulator_handle.hpp | 4 ++-- src/io/simulator/simulator_transport.hpp | 6 +++--- src/io/transport.hpp | 2 +- tests/simulation/basic_request.cpp | 4 ++-- tests/simulation/trial_query_storage/query_storage_test.cpp | 4 ++-- tests/unit/local_transport.cpp | 4 ++-- 9 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/io/local_transport/local_transport.hpp b/src/io/local_transport/local_transport.hpp index a071a7dfa..0b63f8d15 100644 --- a/src/io/local_transport/local_transport.hpp +++ b/src/io/local_transport/local_transport.hpp @@ -31,7 +31,7 @@ class LocalTransport { : local_transport_handle_(std::move(local_transport_handle)) {} template - ResponseFuture Request(Address to_address, Address from_address, RequestT request, + ResponseFuture Request(Address to_address, Address from_address, RequestT &&request, std::function fill_notifier, Duration timeout) { return local_transport_handle_->template SubmitRequest( to_address, from_address, std::move(request), timeout, fill_notifier); @@ -44,7 +44,7 @@ class LocalTransport { template void Send(Address to_address, Address from_address, RequestId request_id, M &&message) { - return local_transport_handle_->template Send(to_address, from_address, request_id, std::forward(message)); + return local_transport_handle_->template Send(to_address, from_address, request_id, std::move(message)); } Time Now() const { return local_transport_handle_->Now(); } diff --git a/src/io/local_transport/local_transport_handle.hpp b/src/io/local_transport/local_transport_handle.hpp index 687ab270f..ed87f1ea9 100644 --- a/src/io/local_transport/local_transport_handle.hpp +++ b/src/io/local_transport/local_transport_handle.hpp @@ -107,7 +107,7 @@ class LocalTransportHandle { void Send(Address to_address, Address from_address, RequestId request_id, M &&message) { auto type_info = TypeInfoFor(message); - std::any message_any(std::forward(message)); + std::any message_any(std::move(message)); OpaqueMessage opaque_message{.to_address = to_address, .from_address = from_address, .request_id = request_id, @@ -168,7 +168,7 @@ class LocalTransportHandle { promises_.emplace(std::move(promise_key), std::move(dop)); } // lock dropped - Send(to_address, from_address, request_id, std::forward(request)); + Send(to_address, from_address, request_id, std::move(request)); return std::move(future); } diff --git a/src/io/rsm/raft.hpp b/src/io/rsm/raft.hpp index 0221eedd0..cbdef4822 100644 --- a/src/io/rsm/raft.hpp +++ b/src/io/rsm/raft.hpp @@ -554,7 +554,7 @@ class Raft { for (const auto &peer : peers_) { // request_id not necessary to set because it's not a Future-backed Request. static constexpr auto request_id = 0; - io_.template Send(peer, request_id, request); + io_.template Send(peer, request_id, VoteRequest{request}); outstanding_votes.insert(peer); } @@ -630,7 +630,7 @@ class Raft { .vote_granted = new_leader, }; - io_.Send(from_address, request_id, res); + io_.Send(from_address, request_id, std::move(res)); if (new_leader) { // become a follower @@ -808,7 +808,7 @@ class Raft { Log("returning log_size of ", res.log_size); - io_.Send(from_address, request_id, res); + io_.Send(from_address, request_id, std::move(res)); return std::nullopt; } diff --git a/src/io/simulator/simulator_handle.hpp b/src/io/simulator/simulator_handle.hpp index 40d28d8c9..acb21eccc 100644 --- a/src/io/simulator/simulator_handle.hpp +++ b/src/io/simulator/simulator_handle.hpp @@ -126,7 +126,7 @@ class SimulatorHandle { const Time deadline = cluster_wide_time_microseconds_ + timeout; - std::any message(request); + std::any message(std::move(request)); OpaqueMessage om{.to_address = to_address, .from_address = from_address, .request_id = request_id, @@ -199,7 +199,7 @@ class SimulatorHandle { auto type_info = TypeInfoFor(message); { std::unique_lock lock(mu_); - std::any message_any(std::forward(message)); + std::any message_any(std::move(message)); OpaqueMessage om{.to_address = to_address, .from_address = from_address, .request_id = request_id, diff --git a/src/io/simulator/simulator_transport.hpp b/src/io/simulator/simulator_transport.hpp index cc7b6f9b0..07e032280 100644 --- a/src/io/simulator/simulator_transport.hpp +++ b/src/io/simulator/simulator_transport.hpp @@ -34,7 +34,7 @@ class SimulatorTransport { : simulator_handle_(simulator_handle), address_(address), rng_(std::mt19937{seed}) {} template - ResponseFuture Request(Address to_address, Address from_address, RequestT request, + ResponseFuture Request(Address to_address, Address from_address, RequestT &&request, std::function notification, Duration timeout) { std::function tick_simulator = [handle_copy = simulator_handle_] { return handle_copy->MaybeTickSimulator(); @@ -50,8 +50,8 @@ class SimulatorTransport { } template - void Send(Address to_address, Address from_address, uint64_t request_id, M message) { - return simulator_handle_->template Send(to_address, from_address, request_id, message); + void Send(Address to_address, Address from_address, uint64_t request_id, M &&message) { + return simulator_handle_->template Send(to_address, from_address, request_id, std::move(message)); } Time Now() const { return simulator_handle_->Now(); } diff --git a/src/io/transport.hpp b/src/io/transport.hpp index bca6250a8..574b69d95 100644 --- a/src/io/transport.hpp +++ b/src/io/transport.hpp @@ -32,7 +32,7 @@ using memgraph::utils::BasicResult; // reasonable constraints around message types over time, // as we adapt things to use Thrift-generated message types. template -concept Message = true; +concept Message = std::movable && std::copyable; using RequestId = uint64_t; diff --git a/tests/simulation/basic_request.cpp b/tests/simulation/basic_request.cpp index 243a57b52..58c9293f2 100644 --- a/tests/simulation/basic_request.cpp +++ b/tests/simulation/basic_request.cpp @@ -55,7 +55,7 @@ void run_server(Io io) { highest_seen = std::max(highest_seen, req.proposal); auto srv_res = CounterResponse{highest_seen}; - io.Send(request_envelope.from_address, request_envelope.request_id, srv_res); + io.Send(request_envelope.from_address, request_envelope.request_id, std::move(srv_res)); } } @@ -76,7 +76,7 @@ std::pair RunWorkload(SimulatorConfig CounterRequest cli_req; cli_req.proposal = i; spdlog::info("[CLIENT] calling Request"); - auto res_f = cli_io.Request(srv_addr, cli_req); + auto res_f = cli_io.Request(srv_addr, std::move(cli_req)); spdlog::info("[CLIENT] calling Wait"); auto res_rez = std::move(res_f).Wait(); spdlog::info("[CLIENT] Wait returned"); diff --git a/tests/simulation/trial_query_storage/query_storage_test.cpp b/tests/simulation/trial_query_storage/query_storage_test.cpp index eb4f86f68..65e617627 100644 --- a/tests/simulation/trial_query_storage/query_storage_test.cpp +++ b/tests/simulation/trial_query_storage/query_storage_test.cpp @@ -44,7 +44,7 @@ void run_server(Io io) { for (auto index = start_index; index < start_index + req.count; ++index) { response.vertices.push_back({std::string("Vertex_") + std::to_string(index)}); } - io.Send(request_envelope.from_address, request_envelope.request_id, response); + io.Send(request_envelope.from_address, request_envelope.request_id, std::move(response)); } } @@ -78,7 +78,7 @@ int main() { auto req = ScanVerticesRequest{2, std::nullopt}; - auto res_f = cli_io.Request(srv_addr, req); + auto res_f = cli_io.Request(srv_addr, std::move(req)); auto res_rez = std::move(res_f).Wait(); simulator.ShutDown(); return 0; diff --git a/tests/unit/local_transport.cpp b/tests/unit/local_transport.cpp index 12ddcd753..45cccb247 100644 --- a/tests/unit/local_transport.cpp +++ b/tests/unit/local_transport.cpp @@ -48,7 +48,7 @@ void RunServer(Io io) { highest_seen = std::max(highest_seen, req.proposal); auto srv_res = CounterResponse{highest_seen}; - io.Send(request_envelope.from_address, request_envelope.request_id, srv_res); + io.Send(request_envelope.from_address, request_envelope.request_id, std::move(srv_res)); } } @@ -70,7 +70,7 @@ TEST(LocalTransport, BasicRequest) { auto value = 1; // i; cli_req.proposal = value; spdlog::info("[CLIENT] sending request"); - auto res_f = cli_io.Request(srv_addr, cli_req); + auto res_f = cli_io.Request(srv_addr, std::move(cli_req)); spdlog::info("[CLIENT] waiting on future"); auto res_rez = std::move(res_f).Wait(); From 545d32722dc66eb32e0ce4574ee2f469e4cee76b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 1 Mar 2023 08:03:15 +0100 Subject: [PATCH 50/56] Do not fetch all the properties by default --- src/query/v2/plan/operator.cpp | 3 +++ src/query/v2/request_router.hpp | 1 + 2 files changed, 4 insertions(+) diff --git a/src/query/v2/plan/operator.cpp b/src/query/v2/plan/operator.cpp index 17ad40c20..39ccf51ef 100644 --- a/src/query/v2/plan/operator.cpp +++ b/src/query/v2/plan/operator.cpp @@ -3122,6 +3122,8 @@ class DistributedExpandCursor : public Cursor { // to not fetch any properties of the edges request.edge_properties.emplace(); request.src_vertices.push_back(vertex.Id()); + request.edge_properties.emplace(); + request.src_vertex_properties.emplace(); auto result_rows = std::invoke([&context, &request]() mutable { SCOPED_REQUEST_WAIT_PROFILE; return context.request_router->ExpandOne(std::move(request)); @@ -3264,6 +3266,7 @@ class DistributedExpandCursor : public Cursor { [](const storage::v3::EdgeTypeId edge_type_id) { return msgs::EdgeType{edge_type_id}; }); // to not fetch any properties of the edges request.edge_properties.emplace(); + request.src_vertex_properties.emplace(); for (const auto &frame : own_multi_frame_->GetValidFramesReader()) { const auto &vertex_value = frame[self_.input_symbol_]; diff --git a/src/query/v2/request_router.hpp b/src/query/v2/request_router.hpp index 290bd2cca..0af4da6f5 100644 --- a/src/query/v2/request_router.hpp +++ b/src/query/v2/request_router.hpp @@ -504,6 +504,7 @@ class RequestRouter : public RequestRouterInterface { msgs::ScanVerticesRequest request; request.transaction_id = transaction_id_; + request.props_to_return.emplace(); request.start_id.second = storage::conversions::ConvertValueVector(key); ShardRequestState shard_request_state{ From ed5aae15ef1e7e1e4177f8657e72314f18cd40b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 1 Mar 2023 10:46:05 +0100 Subject: [PATCH 51/56] Make e2e tests pass --- .../awesome_memgraph_functions.py | 4 ++- tests/e2e/distributed_queries/distinct.py | 11 +++--- .../distributed_expand_one.py | 8 +++-- .../distributed_queries.py | 2 +- .../distributed_queries/order_by_and_limit.py | 36 ++++++++++--------- 5 files changed, 34 insertions(+), 27 deletions(-) diff --git a/tests/e2e/distributed_queries/awesome_memgraph_functions.py b/tests/e2e/distributed_queries/awesome_memgraph_functions.py index 0bdaa07a4..0babec2d3 100644 --- a/tests/e2e/distributed_queries/awesome_memgraph_functions.py +++ b/tests/e2e/distributed_queries/awesome_memgraph_functions.py @@ -36,7 +36,9 @@ def test_awesome_memgraph_functions(connection): assert len(results) == 1 assert results[0][0] == 5 - results = execute_and_fetch_all(cursor, "MATCH (n) WITH COLLECT(n.property) as nn RETURN ALL(i IN nn WHERE i > 0)") + results = execute_and_fetch_all( + cursor, "UNWIND [2, 1, 3] AS value WITH COLLECT(value) as nn RETURN ALL(i IN nn WHERE i > 0)" + ) assert len(results) == 1 assert results[0][0] == True diff --git a/tests/e2e/distributed_queries/distinct.py b/tests/e2e/distributed_queries/distinct.py index 9ebe50e6f..4a8560b03 100644 --- a/tests/e2e/distributed_queries/distinct.py +++ b/tests/e2e/distributed_queries/distinct.py @@ -9,11 +9,13 @@ # by the Apache License, Version 2.0, included in the file # licenses/APL.txt. -import typing -import mgclient import sys -import pytest import time +import typing + +import mgclient +import pytest + from common import * @@ -30,8 +32,7 @@ def test_distinct(connection): assert len(results) == 2 for i, n in enumerate(results): n_props = n[0].properties - assert len(n_props) == 1 - assert n_props["property"] == i + assert len(n_props) == 0 if __name__ == "__main__": diff --git a/tests/e2e/distributed_queries/distributed_expand_one.py b/tests/e2e/distributed_queries/distributed_expand_one.py index 62a09e1e7..1a74daeb1 100644 --- a/tests/e2e/distributed_queries/distributed_expand_one.py +++ b/tests/e2e/distributed_queries/distributed_expand_one.py @@ -27,8 +27,8 @@ def test_sequenced_expand_one(connection): for i in range(1, 4): assert has_n_result_row(cursor, f"CREATE (:label {{property:{i}}})", 0), f"Failed creating node" - assert has_n_result_row(cursor, "MATCH (n {property:1}), (m {property:2}) CREATE (n)-[:TO]->(m)", 0) - assert has_n_result_row(cursor, "MATCH (n {property:2}), (m {property:3}) CREATE (n)-[:TO]->(m)", 0) + assert has_n_result_row(cursor, "MATCH (n:label {property:1}), (m:label {property:2}) CREATE (n)-[:TO]->(m)", 0) + assert has_n_result_row(cursor, "MATCH (n:label {property:2}), (m:label {property:3}) CREATE (n)-[:TO]->(m)", 0) results = execute_and_fetch_all(cursor, "MATCH (n)-[:TO]->(m)-[:TO]->(l) RETURN n,m,l") assert len(results) == 1 @@ -39,7 +39,9 @@ def test_sequenced_expand_one(connection): assert ( len(m.properties) == 0 ), "we don't return any properties of the node received from expansion and the bolt layer doesn't serialize the primary key of vertices" - assert l.properties["property"] == 3 + assert ( + len(l.properties) == 0 + ), "we don't return any properties of the node received from expansion and the bolt layer doesn't serialize the primary key of vertices" if __name__ == "__main__": diff --git a/tests/e2e/distributed_queries/distributed_queries.py b/tests/e2e/distributed_queries/distributed_queries.py index 7b98598a3..460aa517c 100644 --- a/tests/e2e/distributed_queries/distributed_queries.py +++ b/tests/e2e/distributed_queries/distributed_queries.py @@ -43,7 +43,7 @@ def test_vertex_creation_and_scanall(connection): assert r.type == "TO" m_props = m.properties - assert m_props["property"] <= 3 and m_props["property"] >= 0, "Wrong key" + assert len(m_props) == 0, "n is not expected to have properties, update the test!" assert len(m.labels) == 0, "m is not expected to have labels, update the test!" diff --git a/tests/e2e/distributed_queries/order_by_and_limit.py b/tests/e2e/distributed_queries/order_by_and_limit.py index 05297f8f6..29be77857 100644 --- a/tests/e2e/distributed_queries/order_by_and_limit.py +++ b/tests/e2e/distributed_queries/order_by_and_limit.py @@ -9,11 +9,13 @@ # by the Apache License, Version 2.0, included in the file # licenses/APL.txt. -import typing -import mgclient import sys -import pytest import time +import typing + +import mgclient +import pytest + from common import * @@ -21,23 +23,23 @@ def test_order_by_and_limit(connection): wait_for_shard_manager_to_initialize() cursor = connection.cursor() - assert has_n_result_row(cursor, "CREATE (n :label {property:1})", 0) - assert has_n_result_row(cursor, "CREATE (n :label {property:2})", 0) - assert has_n_result_row(cursor, "CREATE (n :label {property:3})", 0) - assert has_n_result_row(cursor, "CREATE (n :label {property:4})", 0) - - results = execute_and_fetch_all(cursor, "MATCH (n) RETURN n ORDER BY n.property DESC") - assert len(results) == 4 - i = 4 - for n in results: - n_props = n[0].properties - assert len(n_props) == 1 - assert n_props["property"] == i + results = execute_and_fetch_all( + cursor, + "UNWIND [{property:1}, {property:3}, {property:2}] AS map RETURN map ORDER BY map.property DESC", + ) + assert len(results) == 3 + i = 3 + for map in results: + assert len(map) == 1 + assert map[0]["property"] == i i = i - 1 - result = execute_and_fetch_all(cursor, "MATCH (n) RETURN n ORDER BY n.property LIMIT 1") + result = execute_and_fetch_all( + cursor, + "UNWIND [{property:1}, {property:3}, {property:2}] AS map RETURN map ORDER BY map.property LIMIT 1", + ) assert len(result) == 1 - assert result[0][0].properties["property"] == 1 + assert result[0][0]["property"] == 1 if __name__ == "__main__": From 13795c89933a95d7985428032c610da9c05fdc95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 1 Mar 2023 08:20:03 +0100 Subject: [PATCH 52/56] Small moves here and there --- src/storage/v3/expr.cpp | 6 +++--- src/storage/v3/request_helper.hpp | 4 ++-- src/storage/v3/shard_rsm.cpp | 12 +++++++----- src/storage/v3/value_conversions.hpp | 13 ++++++++++++- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/src/storage/v3/expr.cpp b/src/storage/v3/expr.cpp index 36c0ff8e3..185e4e29e 100644 --- a/src/storage/v3/expr.cpp +++ b/src/storage/v3/expr.cpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -35,7 +35,7 @@ msgs::Value ConstructValueVertex(const VertexAccessor &acc, View view) { memgraph::msgs::Label value_label{.id = prim_label}; auto prim_key = conversions::ConvertValueVector(acc.PrimaryKey(view).GetValue()); - memgraph::msgs::VertexId vertex_id = std::make_pair(value_label, prim_key); + memgraph::msgs::VertexId vertex_id = std::make_pair(value_label, std::move(prim_key)); // Get the labels auto vertex_labels = acc.Labels(view).GetValue(); @@ -45,7 +45,7 @@ msgs::Value ConstructValueVertex(const VertexAccessor &acc, View view) { std::transform(vertex_labels.begin(), vertex_labels.end(), std::back_inserter(value_labels), [](const auto &label) { return msgs::Label{.id = label}; }); - return msgs::Value({.id = vertex_id, .labels = value_labels}); + return msgs::Value({.id = std::move(vertex_id), .labels = std::move(value_labels)}); } msgs::Value ConstructValueEdge(const EdgeAccessor &acc, View view) { diff --git a/src/storage/v3/request_helper.hpp b/src/storage/v3/request_helper.hpp index bbe4894e9..5590f93ac 100644 --- a/src/storage/v3/request_helper.hpp +++ b/src/storage/v3/request_helper.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -233,7 +233,7 @@ ShardResult> CollectAllPropertiesImpl(const TAccesso [](std::pair &pair) { return std::make_pair(pair.first, conversions::FromPropertyValueToValue(std::move(pair.second))); }); - return ret; + return {std::move(ret)}; } } // namespace impl diff --git a/src/storage/v3/shard_rsm.cpp b/src/storage/v3/shard_rsm.cpp index 881796a70..72ee07f15 100644 --- a/src/storage/v3/shard_rsm.cpp +++ b/src/storage/v3/shard_rsm.cpp @@ -472,7 +472,8 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::ExpandOneRequest &&req) { if (req.order_by_edges.empty()) { const auto *schema = shard_->GetSchema(shard_->PrimaryLabel()); MG_ASSERT(schema); - return GetExpandOneResult(acc, src_vertex, req, maybe_filter_based_on_edge_uniqueness, edge_filler, *schema); + return GetExpandOneResult(acc, std::move(src_vertex), req, maybe_filter_based_on_edge_uniqueness, edge_filler, + *schema); } auto [in_edge_accessors, out_edge_accessors] = GetEdgesFromVertex(src_vertex_acc, req.direction); const auto in_ordered_edges = OrderByEdges(dba, in_edge_accessors, req.order_by_edges, src_vertex_acc); @@ -487,8 +488,9 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::ExpandOneRequest &&req) { [](const auto &edge_element) { return edge_element.object_acc; }); const auto *schema = shard_->GetSchema(shard_->PrimaryLabel()); MG_ASSERT(schema); - return GetExpandOneResult(src_vertex_acc, src_vertex, req, in_edge_ordered_accessors, out_edge_ordered_accessors, - maybe_filter_based_on_edge_uniqueness, edge_filler, *schema); + return GetExpandOneResult(src_vertex_acc, std::move(src_vertex), req, in_edge_ordered_accessors, + out_edge_ordered_accessors, maybe_filter_based_on_edge_uniqueness, edge_filler, + *schema); }); if (maybe_result.HasError()) { @@ -581,12 +583,12 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::GetPropertiesRequest &&req) { if (maybe_id.HasError()) { return {maybe_id.GetError()}; } - const auto &id = maybe_id.GetValue(); + auto &vertex_id = maybe_id.GetValue(); std::optional e_id; if (e_acc) { e_id = msgs::EdgeId{e_acc->Gid().AsUint()}; } - msgs::VertexId v_id{msgs::Label{id.primary_label}, ConvertValueVector(id.primary_key)}; + msgs::VertexId v_id{msgs::Label{vertex_id.primary_label}, ConvertValueVector(std::move(vertex_id.primary_key))}; auto maybe_props = collect_props(v_acc, e_acc); if (maybe_props.HasError()) { return {maybe_props.GetError()}; diff --git a/src/storage/v3/value_conversions.hpp b/src/storage/v3/value_conversions.hpp index 53374e1ed..c068378ed 100644 --- a/src/storage/v3/value_conversions.hpp +++ b/src/storage/v3/value_conversions.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -126,6 +126,17 @@ inline std::vector ConvertValueVector(const std::vector ConvertValueVector(std::vector &&vec) { + std::vector ret; + ret.reserve(vec.size()); + + for (auto &&elem : vec) { + ret.push_back(FromPropertyValueToValue(std::move(elem))); + } + + return ret; +} + inline msgs::VertexId ToMsgsVertexId(const v3::VertexId &vertex_id) { return {msgs::Label{vertex_id.primary_label}, ConvertValueVector(vertex_id.primary_key)}; } From 53cd5592e0d86b748dad99f494a9369b0dd7e47d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Wed, 1 Mar 2023 16:39:32 +0100 Subject: [PATCH 53/56] Enforce move in transport layer --- src/io/local_transport/local_transport.hpp | 6 ++-- .../local_transport_handle.hpp | 8 ++--- src/io/rsm/raft.hpp | 20 ++++++++----- src/io/rsm/rsm_client.hpp | 15 +++++----- src/io/simulator/simulator_handle.hpp | 16 +++++----- src/io/simulator/simulator_transport.hpp | 10 +++---- src/io/transport.hpp | 29 ++++++++++++------- src/query/v2/request_router.hpp | 5 ++-- src/storage/v3/shard_manager.hpp | 5 ++-- src/utils/concepts.hpp | 6 +++- tests/simulation/basic_request.cpp | 2 +- .../query_storage_test.cpp | 2 +- tests/unit/local_transport.cpp | 2 +- 13 files changed, 73 insertions(+), 53 deletions(-) diff --git a/src/io/local_transport/local_transport.hpp b/src/io/local_transport/local_transport.hpp index 0b63f8d15..f17e5c059 100644 --- a/src/io/local_transport/local_transport.hpp +++ b/src/io/local_transport/local_transport.hpp @@ -31,7 +31,7 @@ class LocalTransport { : local_transport_handle_(std::move(local_transport_handle)) {} template - ResponseFuture Request(Address to_address, Address from_address, RequestT &&request, + ResponseFuture Request(Address to_address, Address from_address, RValueRef request, std::function fill_notifier, Duration timeout) { return local_transport_handle_->template SubmitRequest( to_address, from_address, std::move(request), timeout, fill_notifier); @@ -43,8 +43,8 @@ class LocalTransport { } template - void Send(Address to_address, Address from_address, RequestId request_id, M &&message) { - return local_transport_handle_->template Send(to_address, from_address, request_id, std::move(message)); + void Send(Address to_address, Address from_address, RequestId request_id, RValueRef message) { + return local_transport_handle_->template Send(to_address, from_address, request_id, std::move(message)); } Time Now() const { return local_transport_handle_->Now(); } diff --git a/src/io/local_transport/local_transport_handle.hpp b/src/io/local_transport/local_transport_handle.hpp index ed87f1ea9..cd19a3977 100644 --- a/src/io/local_transport/local_transport_handle.hpp +++ b/src/io/local_transport/local_transport_handle.hpp @@ -104,7 +104,7 @@ class LocalTransportHandle { } template - void Send(Address to_address, Address from_address, RequestId request_id, M &&message) { + void Send(Address to_address, Address from_address, RequestId request_id, RValueRef message) { auto type_info = TypeInfoFor(message); std::any message_any(std::move(message)); @@ -139,13 +139,13 @@ class LocalTransportHandle { } template - ResponseFuture SubmitRequest(Address to_address, Address from_address, RequestT &&request, + ResponseFuture SubmitRequest(Address to_address, Address from_address, RValueRef request, Duration timeout, std::function fill_notifier) { auto [future, promise] = memgraph::io::FuturePromisePairWithNotifications>( // set null notifier for when the Future::Wait is called nullptr, // set notifier for when Promise::Fill is called - std::forward>(fill_notifier)); + std::move(fill_notifier)); const bool port_matches = to_address.last_known_port == from_address.last_known_port; const bool ip_matches = to_address.last_known_ip == from_address.last_known_ip; @@ -168,7 +168,7 @@ class LocalTransportHandle { promises_.emplace(std::move(promise_key), std::move(dop)); } // lock dropped - Send(to_address, from_address, request_id, std::move(request)); + Send(to_address, from_address, request_id, std::move(request)); return std::move(future); } diff --git a/src/io/rsm/raft.hpp b/src/io/rsm/raft.hpp index cbdef4822..3be15cc46 100644 --- a/src/io/rsm/raft.hpp +++ b/src/io/rsm/raft.hpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -624,13 +625,12 @@ class Raft { MG_ASSERT(std::max(req.term, state_.term) == req.term); } - VoteResponse res{ - .term = std::max(req.term, state_.term), - .committed_log_size = state_.committed_log_size, - .vote_granted = new_leader, - }; - - io_.Send(from_address, request_id, std::move(res)); + io_.Send(from_address, request_id, + VoteResponse{ + .term = std::max(req.term, state_.term), + .committed_log_size = state_.committed_log_size, + .vote_granted = new_leader, + }); if (new_leader) { // become a follower @@ -718,6 +718,10 @@ class Raft { .log_size = state_.log.size(), }; + static_assert(std::is_trivially_copyable_v, + "This function copies this message, therefore it is important to be trivially copyable. Otherwise it " + "should be moved"); + if constexpr (std::is_same()) { MG_ASSERT(req.term != state_.term, "Multiple leaders are acting under the term ", req.term); } @@ -808,7 +812,7 @@ class Raft { Log("returning log_size of ", res.log_size); - io_.Send(from_address, request_id, std::move(res)); + io_.Send(from_address, request_id, AppendResponse{res}); return std::nullopt; } diff --git a/src/io/rsm/rsm_client.hpp b/src/io/rsm/rsm_client.hpp index a98a1dcd2..bc5dc6a47 100644 --- a/src/io/rsm/rsm_client.hpp +++ b/src/io/rsm/rsm_client.hpp @@ -122,8 +122,8 @@ class RsmClient { .start_time = io_.Now(), .request = std::move(req), .notifier = notifier, - .future = io_.template RequestWithNotification>(leader_, std::move(read_req), - notifier, readiness_token), + .future = io_.template RequestWithNotification, ReadRequest>( + leader_, std::move(read_req), notifier, readiness_token), }; async_reads_.emplace(readiness_token.GetId(), std::move(async_request)); @@ -134,7 +134,7 @@ class RsmClient { ReadRequest read_req = {.operation = async_request.request}; - async_request.future = io_.template RequestWithNotification>( + async_request.future = io_.template RequestWithNotification, ReadRequest>( leader_, std::move(read_req), async_request.notifier, readiness_token); } @@ -191,8 +191,8 @@ class RsmClient { .start_time = io_.Now(), .request = std::move(req), .notifier = notifier, - .future = io_.template RequestWithNotification>(leader_, std::move(write_req), - notifier, readiness_token), + .future = io_.template RequestWithNotification, WriteRequest>( + leader_, std::move(write_req), notifier, readiness_token), }; async_writes_.emplace(readiness_token.GetId(), std::move(async_request)); @@ -203,8 +203,9 @@ class RsmClient { WriteRequest write_req = {.operation = async_request.request}; - async_request.future = io_.template RequestWithNotification>( - leader_, std::move(write_req), async_request.notifier, readiness_token); + async_request.future = + io_.template RequestWithNotification, WriteRequest>( + leader_, std::move(write_req), async_request.notifier, readiness_token); } std::optional> PollAsyncWriteRequest(const ReadinessToken &readiness_token) { diff --git a/src/io/simulator/simulator_handle.hpp b/src/io/simulator/simulator_handle.hpp index acb21eccc..a34e93c66 100644 --- a/src/io/simulator/simulator_handle.hpp +++ b/src/io/simulator/simulator_handle.hpp @@ -105,19 +105,19 @@ class SimulatorHandle { bool ShouldShutDown() const; - template - ResponseFuture SubmitRequest(Address to_address, Address from_address, Request &&request, Duration timeout, - std::function &&maybe_tick_simulator, - std::function &&fill_notifier) { + template + ResponseFuture SubmitRequest(Address to_address, Address from_address, RValueRef request, + Duration timeout, std::function &&maybe_tick_simulator, + std::function &&fill_notifier) { auto type_info = TypeInfoFor(request); std::string demangled_name = boost::core::demangle(type_info.get().name()); spdlog::trace("simulator sending request {} to {}", demangled_name, to_address); - auto [future, promise] = memgraph::io::FuturePromisePairWithNotifications>( + auto [future, promise] = memgraph::io::FuturePromisePairWithNotifications>( // set notifier for when the Future::Wait is called - std::forward>(maybe_tick_simulator), + std::move(maybe_tick_simulator), // set notifier for when Promise::Fill is called - std::forward>(fill_notifier)); + std::move(fill_notifier)); { std::unique_lock lock(mu_); @@ -194,7 +194,7 @@ class SimulatorHandle { } template - void Send(Address to_address, Address from_address, RequestId request_id, M &&message) { + void Send(Address to_address, Address from_address, RequestId request_id, RValueRef message) { spdlog::trace("sending message from {} to {}", from_address.last_known_port, to_address.last_known_port); auto type_info = TypeInfoFor(message); { diff --git a/src/io/simulator/simulator_transport.hpp b/src/io/simulator/simulator_transport.hpp index 07e032280..05775e726 100644 --- a/src/io/simulator/simulator_transport.hpp +++ b/src/io/simulator/simulator_transport.hpp @@ -34,14 +34,14 @@ class SimulatorTransport { : simulator_handle_(simulator_handle), address_(address), rng_(std::mt19937{seed}) {} template - ResponseFuture Request(Address to_address, Address from_address, RequestT &&request, + ResponseFuture Request(Address to_address, Address from_address, RValueRef request, std::function notification, Duration timeout) { std::function tick_simulator = [handle_copy = simulator_handle_] { return handle_copy->MaybeTickSimulator(); }; - return simulator_handle_->template SubmitRequest(to_address, from_address, std::move(request), timeout, - std::move(tick_simulator), std::move(notification)); + return simulator_handle_->template SubmitRequest( + to_address, from_address, std::move(request), timeout, std::move(tick_simulator), std::move(notification)); } template @@ -50,8 +50,8 @@ class SimulatorTransport { } template - void Send(Address to_address, Address from_address, uint64_t request_id, M &&message) { - return simulator_handle_->template Send(to_address, from_address, request_id, std::move(message)); + void Send(Address to_address, Address from_address, uint64_t request_id, RValueRef message) { + return simulator_handle_->template Send(to_address, from_address, request_id, std::move(message)); } Time Now() const { return simulator_handle_->Now(); } diff --git a/src/io/transport.hpp b/src/io/transport.hpp index 574b69d95..eade62db3 100644 --- a/src/io/transport.hpp +++ b/src/io/transport.hpp @@ -22,6 +22,7 @@ #include "io/message_histogram_collector.hpp" #include "io/notifier.hpp" #include "io/time.hpp" +#include "utils/concepts.hpp" #include "utils/result.hpp" namespace memgraph::io { @@ -34,6 +35,14 @@ using memgraph::utils::BasicResult; template concept Message = std::movable && std::copyable; +template +struct RValueRefEnforcer { + using Type = T &&; +}; + +template +using RValueRef = typename RValueRefEnforcer::Type; + using RequestId = uint64_t; template @@ -83,33 +92,33 @@ class Io { /// Issue a request with an explicit timeout in microseconds provided. This tends to be used by clients. template - ResponseFuture RequestWithTimeout(Address address, RequestT &&request, Duration timeout) { + ResponseFuture RequestWithTimeout(Address address, RValueRef request, Duration timeout) { const Address from_address = address_; std::function fill_notifier = nullptr; - return implementation_.template Request(address, from_address, std::forward(request), - fill_notifier, timeout); + return implementation_.template Request(address, from_address, std::move(request), + fill_notifier, timeout); } /// Issue a request that times out after the default timeout. This tends /// to be used by clients. template - ResponseFuture Request(Address to_address, RequestT &&request) { + ResponseFuture Request(Address to_address, RValueRef request) { const Duration timeout = default_timeout_; const Address from_address = address_; std::function fill_notifier = nullptr; - return implementation_.template Request(to_address, from_address, std::forward(request), - fill_notifier, timeout); + return implementation_.template Request(to_address, from_address, std::move(request), + fill_notifier, timeout); } /// Issue a request that will notify a Notifier when it is filled or times out. template - ResponseFuture RequestWithNotification(Address to_address, RequestT &&request, Notifier notifier, + ResponseFuture RequestWithNotification(Address to_address, RValueRef request, Notifier notifier, ReadinessToken readiness_token) { const Duration timeout = default_timeout_; const Address from_address = address_; std::function fill_notifier = [notifier, readiness_token]() { notifier.Notify(readiness_token); }; - return implementation_.template Request(to_address, from_address, std::forward(request), - fill_notifier, timeout); + return implementation_.template Request(to_address, from_address, std::move(request), + fill_notifier, timeout); } /// Issue a request that will notify a Notifier when it is filled or times out. @@ -143,7 +152,7 @@ class Io { template void Send(Address to_address, RequestId request_id, M &&message) { Address from_address = address_; - return implementation_.template Send(to_address, from_address, request_id, std::forward(message)); + return implementation_.template Send(to_address, from_address, request_id, std::forward(message)); } /// The current system time. This time source should be preferred over any other, diff --git a/src/query/v2/request_router.hpp b/src/query/v2/request_router.hpp index 0af4da6f5..04107a73e 100644 --- a/src/query/v2/request_router.hpp +++ b/src/query/v2/request_router.hpp @@ -729,8 +729,9 @@ class RequestRouter : public RequestRouterInterface { io::rsm::WriteRequest ww; ww.operation = std::move(requests); - auto resp = io_.template Request>( - coordinator_address, std::move(ww)) + auto resp = io_.template Request, + io::rsm::WriteRequest>(coordinator_address, + std::move(ww)) .Wait(); if (resp.HasValue()) { const auto alloc_edge_id_reps = diff --git a/src/storage/v3/shard_manager.hpp b/src/storage/v3/shard_manager.hpp index 03cdf7159..ae7a5f4ef 100644 --- a/src/storage/v3/shard_manager.hpp +++ b/src/storage/v3/shard_manager.hpp @@ -249,8 +249,9 @@ class ShardManager { ww.operation = cwr; spdlog::info("SM sending heartbeat to coordinator {}", coordinator_leader_.ToString()); - heartbeat_res_.emplace( - std::move(io_.template Request>(coordinator_leader_, std::move(ww)))); + heartbeat_res_.emplace(std::move( + io_.template Request, WriteRequest>( + coordinator_leader_, std::move(ww)))); spdlog::info("SM sent heartbeat"); } diff --git a/src/utils/concepts.hpp b/src/utils/concepts.hpp index 7c1f3a9c8..9ffc04642 100644 --- a/src/utils/concepts.hpp +++ b/src/utils/concepts.hpp @@ -1,4 +1,4 @@ -// Copyright 2022 Memgraph Ltd. +// Copyright 2023 Memgraph Ltd. // // Use of this software is governed by the Business Source License // included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source @@ -12,6 +12,7 @@ #pragma once #include #include +#include namespace memgraph::utils { template @@ -34,4 +35,7 @@ template concept Dereferenceable = requires(T t) { { *t } -> CanReference; }; + +template +concept Object = std::is_object_v; } // namespace memgraph::utils diff --git a/tests/simulation/basic_request.cpp b/tests/simulation/basic_request.cpp index 58c9293f2..d6504a365 100644 --- a/tests/simulation/basic_request.cpp +++ b/tests/simulation/basic_request.cpp @@ -76,7 +76,7 @@ std::pair RunWorkload(SimulatorConfig CounterRequest cli_req; cli_req.proposal = i; spdlog::info("[CLIENT] calling Request"); - auto res_f = cli_io.Request(srv_addr, std::move(cli_req)); + auto res_f = cli_io.Request(srv_addr, std::move(cli_req)); spdlog::info("[CLIENT] calling Wait"); auto res_rez = std::move(res_f).Wait(); spdlog::info("[CLIENT] Wait returned"); diff --git a/tests/simulation/trial_query_storage/query_storage_test.cpp b/tests/simulation/trial_query_storage/query_storage_test.cpp index 65e617627..9e778f59d 100644 --- a/tests/simulation/trial_query_storage/query_storage_test.cpp +++ b/tests/simulation/trial_query_storage/query_storage_test.cpp @@ -78,7 +78,7 @@ int main() { auto req = ScanVerticesRequest{2, std::nullopt}; - auto res_f = cli_io.Request(srv_addr, std::move(req)); + auto res_f = cli_io.Request(srv_addr, std::move(req)); auto res_rez = std::move(res_f).Wait(); simulator.ShutDown(); return 0; diff --git a/tests/unit/local_transport.cpp b/tests/unit/local_transport.cpp index 45cccb247..f2d3f8458 100644 --- a/tests/unit/local_transport.cpp +++ b/tests/unit/local_transport.cpp @@ -70,7 +70,7 @@ TEST(LocalTransport, BasicRequest) { auto value = 1; // i; cli_req.proposal = value; spdlog::info("[CLIENT] sending request"); - auto res_f = cli_io.Request(srv_addr, std::move(cli_req)); + auto res_f = cli_io.Request(srv_addr, std::move(cli_req)); spdlog::info("[CLIENT] waiting on future"); auto res_rez = std::move(res_f).Wait(); From ab52d325e0f99fbf2bfd6be9ae4d85bbbb92ff64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Thu, 2 Mar 2023 11:22:36 +0100 Subject: [PATCH 54/56] Print and save response latencies --- tests/manual/query_performance.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/manual/query_performance.cpp b/tests/manual/query_performance.cpp index b766d5fbb..8c7877b0d 100644 --- a/tests/manual/query_performance.cpp +++ b/tests/manual/query_performance.cpp @@ -74,6 +74,7 @@ #include #include +#include #include #include #include @@ -320,6 +321,8 @@ void RunV3() { std::chrono::duration_cast(benchmark_end - benchmark_start).count()); ls.ShutDown(); + auto latency_histograms = nlohmann::json::parse(fmt::format("{}", io.ResponseLatencies())); + spdlog::warn(latency_histograms.dump(4)); if (!FLAGS_export_json_results.empty()) { nlohmann::json results; @@ -329,6 +332,7 @@ void RunV3() { PutResult(benchmark_results_json, name, duration); } results["benchmarks"] = std::move(benchmark_results_json); + results["latencies"] = std::move(latency_histograms); std::ofstream results_file{FLAGS_export_json_results}; results_file << results.dump(); } From f1b175787b8408ff29b3d361331c7d103b3e24f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Fri, 3 Mar 2023 07:46:43 +0100 Subject: [PATCH 55/56] Reduce number of `FindVertex` calls --- src/storage/v3/request_helper.cpp | 5 +---- src/storage/v3/request_helper.hpp | 2 +- src/storage/v3/shard_rsm.cpp | 4 ++-- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/storage/v3/request_helper.cpp b/src/storage/v3/request_helper.cpp index f5d72ed15..8e20d8e58 100644 --- a/src/storage/v3/request_helper.cpp +++ b/src/storage/v3/request_helper.cpp @@ -383,13 +383,10 @@ bool FilterOnEdge(DbAccessor &dba, const storage::v3::VertexAccessor &v_acc, con } ShardResult GetExpandOneResult( - Shard::Accessor &acc, msgs::VertexId src_vertex, const msgs::ExpandOneRequest &req, + VertexAccessor v_acc, msgs::VertexId src_vertex, const msgs::ExpandOneRequest &req, const EdgeUniquenessFunction &maybe_filter_based_on_edge_uniqueness, const EdgeFiller &edge_filler, const Schemas::Schema &schema) { /// Fill up source vertex - const auto primary_key = ConvertPropertyVector(src_vertex.second); - auto v_acc = acc.FindVertex(primary_key, View::NEW); - msgs::Vertex source_vertex = {.id = src_vertex}; auto maybe_secondary_labels = FillUpSourceVertexSecondaryLabels(v_acc, req); if (maybe_secondary_labels.HasError()) { diff --git a/src/storage/v3/request_helper.hpp b/src/storage/v3/request_helper.hpp index 5590f93ac..37391cb3d 100644 --- a/src/storage/v3/request_helper.hpp +++ b/src/storage/v3/request_helper.hpp @@ -247,7 +247,7 @@ EdgeUniquenessFunction InitializeEdgeUniquenessFunction(bool only_unique_neighbo EdgeFiller InitializeEdgeFillerFunction(const msgs::ExpandOneRequest &req); ShardResult GetExpandOneResult( - Shard::Accessor &acc, msgs::VertexId src_vertex, const msgs::ExpandOneRequest &req, + VertexAccessor v_acc, msgs::VertexId src_vertex, const msgs::ExpandOneRequest &req, const EdgeUniquenessFunction &maybe_filter_based_on_edge_uniqueness, const EdgeFiller &edge_filler, const Schemas::Schema &schema); diff --git a/src/storage/v3/shard_rsm.cpp b/src/storage/v3/shard_rsm.cpp index 72ee07f15..fc0192973 100644 --- a/src/storage/v3/shard_rsm.cpp +++ b/src/storage/v3/shard_rsm.cpp @@ -472,8 +472,8 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::ExpandOneRequest &&req) { if (req.order_by_edges.empty()) { const auto *schema = shard_->GetSchema(shard_->PrimaryLabel()); MG_ASSERT(schema); - return GetExpandOneResult(acc, std::move(src_vertex), req, maybe_filter_based_on_edge_uniqueness, edge_filler, - *schema); + return GetExpandOneResult(src_vertex_acc, std::move(src_vertex), req, maybe_filter_based_on_edge_uniqueness, + edge_filler, *schema); } auto [in_edge_accessors, out_edge_accessors] = GetEdgesFromVertex(src_vertex_acc, req.direction); const auto in_ordered_edges = OrderByEdges(dba, in_edge_accessors, req.order_by_edges, src_vertex_acc); From 89b43a45c973c14991248a357faf2de785e8213f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A1nos=20Benjamin=20Antal?= Date: Fri, 3 Mar 2023 07:47:02 +0100 Subject: [PATCH 56/56] Move edges container --- src/storage/v3/shard_rsm.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/storage/v3/shard_rsm.cpp b/src/storage/v3/shard_rsm.cpp index fc0192973..6f71c7269 100644 --- a/src/storage/v3/shard_rsm.cpp +++ b/src/storage/v3/shard_rsm.cpp @@ -488,13 +488,13 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::ExpandOneRequest &&req) { [](const auto &edge_element) { return edge_element.object_acc; }); const auto *schema = shard_->GetSchema(shard_->PrimaryLabel()); MG_ASSERT(schema); - return GetExpandOneResult(src_vertex_acc, std::move(src_vertex), req, in_edge_ordered_accessors, - out_edge_ordered_accessors, maybe_filter_based_on_edge_uniqueness, edge_filler, - *schema); + return GetExpandOneResult(src_vertex_acc, std::move(src_vertex), req, std::move(in_edge_ordered_accessors), + std::move(out_edge_ordered_accessors), maybe_filter_based_on_edge_uniqueness, + edge_filler, *schema); }); if (maybe_result.HasError()) { - shard_error.emplace(CreateErrorResponse(primary_key.GetError(), req.transaction_id, "getting primary key")); + shard_error.emplace(CreateErrorResponse(maybe_result.GetError(), req.transaction_id, "getting expand result")); break; }