From 3e90f3bea5e489d7e57b69557a3c64849a4f7348 Mon Sep 17 00:00:00 2001 From: Na-Na Pang Date: Fri, 25 Oct 2019 11:32:08 -0700 Subject: [PATCH 1/7] fix result inaccuracy in driver.cc --- test/cpp/qps/driver.cc | 70 +++++++++++++++++++++++++----------------- 1 file changed, 41 insertions(+), 29 deletions(-) diff --git a/test/cpp/qps/driver.cc b/test/cpp/qps/driver.cc index c37529ea274..69f6fd0c959 100644 --- a/test/cpp/qps/driver.cc +++ b/test/cpp/qps/driver.cc @@ -463,6 +463,7 @@ std::unique_ptr RunScenario( gpr_timer_set_enabled(0); // Finish a run + // Finish clients std::unique_ptr result(new ScenarioResult); Histogram merged_latencies; std::unordered_map merged_statuses; @@ -477,17 +478,9 @@ std::unique_ptr RunScenario( gpr_log(GPR_ERROR, "Failed WritesDone for client %zu", i); } } - gpr_log(GPR_INFO, "Finishing servers"); - for (size_t i = 0; i < num_servers; i++) { - auto server = &servers[i]; - if (!server->stream->Write(server_mark)) { - gpr_log(GPR_ERROR, "Couldn't write mark to server %zu", i); - } - if (!server->stream->WritesDone()) { - gpr_log(GPR_ERROR, "Failed WritesDone for server %zu", i); - } - } + // Collect the client final run results before finish server + // otherwise, we will include client shutdown process in benchmark results for (size_t i = 0; i < num_clients; i++) { auto client = &clients[i]; // Read the client final status @@ -506,28 +499,21 @@ std::unique_ptr RunScenario( gpr_log(GPR_ERROR, "Couldn't get final status from client %zu", i); } } - for (size_t i = 0; i < num_clients; i++) { - auto client = &clients[i]; - Status s = client->stream->Finish(); - // Since we shutdown servers and clients at the same time, clients can - // observe cancellation. Thus, we consider both OK and CANCELLED as good - // status. - const bool success = IsSuccess(s); - result->add_client_success(success); - if (!success) { - gpr_log(GPR_ERROR, "Client %zu had an error %s", i, - s.error_message().c_str()); - } - } - merged_latencies.FillProto(result->mutable_latencies()); - for (std::unordered_map::iterator it = merged_statuses.begin(); - it != merged_statuses.end(); ++it) { - RequestResultCount* rrc = result->add_request_results(); - rrc->set_status_code(it->first); - rrc->set_count(it->second); + // Finish servers + gpr_log(GPR_INFO, "Finishing servers"); + for (size_t i = 0; i < num_servers; i++) { + auto server = &servers[i]; + if (!server->stream->Write(server_mark)) { + gpr_log(GPR_ERROR, "Couldn't write mark to server %zu", i); + } + if (!server->stream->WritesDone()) { + gpr_log(GPR_ERROR, "Failed WritesDone for server %zu", i); + } } + // Collect the server final run results before checking client status + // otherwise, we will wait for the benchmark for (size_t i = 0; i < num_servers; i++) { auto server = &servers[i]; // Read the server final status @@ -541,6 +527,23 @@ std::unique_ptr RunScenario( gpr_log(GPR_ERROR, "Couldn't get final status from server %zu", i); } } + + // Get final rpc status from clients + for (size_t i = 0; i < num_clients; i++) { + auto client = &clients[i]; + Status s = client->stream->Finish(); + // Since we shutdown servers and clients at the same time, clients can + // observe cancellation. Thus, we consider both OK and CANCELLED as good + // status. + const bool success = IsSuccess(s); + result->add_client_success(success); + if (!success) { + gpr_log(GPR_ERROR, "Client %zu had an error %s", i, + s.error_message().c_str()); + } + } + + // Get final rpc status from servers for (size_t i = 0; i < num_servers; i++) { auto server = &servers[i]; Status s = server->stream->Finish(); @@ -558,6 +561,15 @@ std::unique_ptr RunScenario( if (g_inproc_servers != nullptr) { delete g_inproc_servers; } + + // Post-processing the results summary + merged_latencies.FillProto(result->mutable_latencies()); + for (std::unordered_map::iterator it = merged_statuses.begin(); + it != merged_statuses.end(); ++it) { + RequestResultCount* rrc = result->add_request_results(); + rrc->set_status_code(it->first); + rrc->set_count(it->second); + } postprocess_scenario_result(result.get()); return result; } From 8edf0dd0c8fa76a320b135ec36c2345d98a91c4d Mon Sep 17 00:00:00 2001 From: Na-Na Pang Date: Fri, 25 Oct 2019 11:56:27 -0700 Subject: [PATCH 2/7] clang format --- test/cpp/qps/driver.cc | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/test/cpp/qps/driver.cc b/test/cpp/qps/driver.cc index 69f6fd0c959..2ca0c081e4c 100644 --- a/test/cpp/qps/driver.cc +++ b/test/cpp/qps/driver.cc @@ -479,8 +479,7 @@ std::unique_ptr RunScenario( } } - // Collect the client final run results before finish server - // otherwise, we will include client shutdown process in benchmark results + // Collect the client final run results after clients stop sending new rpcs for (size_t i = 0; i < num_clients; i++) { auto client = &clients[i]; // Read the client final status @@ -512,8 +511,7 @@ std::unique_ptr RunScenario( } } - // Collect the server final run results before checking client status - // otherwise, we will wait for the benchmark + // Collect the server final run results after servers stop receiving rpcs for (size_t i = 0; i < num_servers; i++) { auto server = &servers[i]; // Read the server final status @@ -527,7 +525,7 @@ std::unique_ptr RunScenario( gpr_log(GPR_ERROR, "Couldn't get final status from server %zu", i); } } - + // Get final rpc status from clients for (size_t i = 0; i < num_clients; i++) { auto client = &clients[i]; @@ -542,7 +540,7 @@ std::unique_ptr RunScenario( s.error_message().c_str()); } } - + // Get final rpc status from servers for (size_t i = 0; i < num_servers; i++) { auto server = &servers[i]; From b6fe2678bbd929d5434a708364af54732dce3182 Mon Sep 17 00:00:00 2001 From: Na-Na Pang Date: Fri, 25 Oct 2019 12:49:46 -0700 Subject: [PATCH 3/7] Clarify comments --- test/cpp/qps/driver.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/cpp/qps/driver.cc b/test/cpp/qps/driver.cc index 2ca0c081e4c..4178715fa8f 100644 --- a/test/cpp/qps/driver.cc +++ b/test/cpp/qps/driver.cc @@ -463,7 +463,6 @@ std::unique_ptr RunScenario( gpr_timer_set_enabled(0); // Finish a run - // Finish clients std::unique_ptr result(new ScenarioResult); Histogram merged_latencies; std::unordered_map merged_statuses; @@ -479,7 +478,7 @@ std::unique_ptr RunScenario( } } - // Collect the client final run results after clients stop sending new rpcs + // Collect the client final run results right after finishing clients for (size_t i = 0; i < num_clients; i++) { auto client = &clients[i]; // Read the client final status @@ -499,7 +498,6 @@ std::unique_ptr RunScenario( } } - // Finish servers gpr_log(GPR_INFO, "Finishing servers"); for (size_t i = 0; i < num_servers; i++) { auto server = &servers[i]; @@ -511,7 +509,7 @@ std::unique_ptr RunScenario( } } - // Collect the server final run results after servers stop receiving rpcs + // Collect the server final run results right after finishing server for (size_t i = 0; i < num_servers; i++) { auto server = &servers[i]; // Read the server final status From 968f7130a790a0919714bb6e6e509cd95348be33 Mon Sep 17 00:00:00 2001 From: Na-Na Pang Date: Fri, 25 Oct 2019 15:27:34 -0700 Subject: [PATCH 4/7] Fix typo --- test/cpp/qps/driver.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/cpp/qps/driver.cc b/test/cpp/qps/driver.cc index 4178715fa8f..79f104f47ae 100644 --- a/test/cpp/qps/driver.cc +++ b/test/cpp/qps/driver.cc @@ -478,7 +478,7 @@ std::unique_ptr RunScenario( } } - // Collect the client final run results right after finishing clients + // Collect clients' final run results right after finishing clients for (size_t i = 0; i < num_clients; i++) { auto client = &clients[i]; // Read the client final status @@ -509,7 +509,7 @@ std::unique_ptr RunScenario( } } - // Collect the server final run results right after finishing server + // Collect servers' final run results right after finishing server for (size_t i = 0; i < num_servers; i++) { auto server = &servers[i]; // Read the server final status From 1ead5c19057c9ea09d5134bba5a9cae42d14d555 Mon Sep 17 00:00:00 2001 From: Na-Na Pang Date: Mon, 28 Oct 2019 10:00:25 -0700 Subject: [PATCH 5/7] remove the last invalid read, which cause hanging issue in driver --- test/cpp/qps/driver.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/cpp/qps/driver.cc b/test/cpp/qps/driver.cc index 79f104f47ae..2a850da6c60 100644 --- a/test/cpp/qps/driver.cc +++ b/test/cpp/qps/driver.cc @@ -491,8 +491,6 @@ std::unique_ptr RunScenario( stats.request_results(i).count(); } result->add_client_stats()->CopyFrom(stats); - // That final status should be the last message on the client stream - GPR_ASSERT(!client->stream->Read(&client_status)); } else { gpr_log(GPR_ERROR, "Couldn't get final status from client %zu", i); } From 03ac8490ea01862b9bfa8f7094b1c63f12b2b81e Mon Sep 17 00:00:00 2001 From: Na-Na Pang Date: Mon, 28 Oct 2019 10:20:21 -0700 Subject: [PATCH 6/7] revert changes --- test/cpp/qps/driver.cc | 50 ++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/test/cpp/qps/driver.cc b/test/cpp/qps/driver.cc index 2a850da6c60..b745f741c49 100644 --- a/test/cpp/qps/driver.cc +++ b/test/cpp/qps/driver.cc @@ -491,6 +491,8 @@ std::unique_ptr RunScenario( stats.request_results(i).count(); } result->add_client_stats()->CopyFrom(stats); + // That final status should be the last message on the client stream + // GPR_ASSERT(!client->stream->Read(&client_status)); } else { gpr_log(GPR_ERROR, "Couldn't get final status from client %zu", i); } @@ -507,21 +509,6 @@ std::unique_ptr RunScenario( } } - // Collect servers' final run results right after finishing server - for (size_t i = 0; i < num_servers; i++) { - auto server = &servers[i]; - // Read the server final status - if (server->stream->Read(&server_status)) { - gpr_log(GPR_INFO, "Received final status from server %zu", i); - result->add_server_stats()->CopyFrom(server_status.stats()); - result->add_server_cores(server_status.cores()); - // That final status should be the last message on the server stream - GPR_ASSERT(!server->stream->Read(&server_status)); - } else { - gpr_log(GPR_ERROR, "Couldn't get final status from server %zu", i); - } - } - // Get final rpc status from clients for (size_t i = 0; i < num_clients; i++) { auto client = &clients[i]; @@ -537,6 +524,30 @@ std::unique_ptr RunScenario( } } + // Post-processing the results summary + merged_latencies.FillProto(result->mutable_latencies()); + for (std::unordered_map::iterator it = merged_statuses.begin(); + it != merged_statuses.end(); ++it) { + RequestResultCount* rrc = result->add_request_results(); + rrc->set_status_code(it->first); + rrc->set_count(it->second); + } + + // Collect servers' final run results right after finishing server + for (size_t i = 0; i < num_servers; i++) { + auto server = &servers[i]; + // Read the server final status + if (server->stream->Read(&server_status)) { + gpr_log(GPR_INFO, "Received final status from server %zu", i); + result->add_server_stats()->CopyFrom(server_status.stats()); + result->add_server_cores(server_status.cores()); + // That final status should be the last message on the server stream + GPR_ASSERT(!server->stream->Read(&server_status)); + } else { + gpr_log(GPR_ERROR, "Couldn't get final status from server %zu", i); + } + } + // Get final rpc status from servers for (size_t i = 0; i < num_servers; i++) { auto server = &servers[i]; @@ -555,15 +566,6 @@ std::unique_ptr RunScenario( if (g_inproc_servers != nullptr) { delete g_inproc_servers; } - - // Post-processing the results summary - merged_latencies.FillProto(result->mutable_latencies()); - for (std::unordered_map::iterator it = merged_statuses.begin(); - it != merged_statuses.end(); ++it) { - RequestResultCount* rrc = result->add_request_results(); - rrc->set_status_code(it->first); - rrc->set_count(it->second); - } postprocess_scenario_result(result.get()); return result; } From 18580a9f4e5cd1698f95bd53fdeca21e82a97e16 Mon Sep 17 00:00:00 2001 From: Na-Na Pang Date: Mon, 28 Oct 2019 10:44:42 -0700 Subject: [PATCH 7/7] clang format --- test/cpp/qps/driver.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cpp/qps/driver.cc b/test/cpp/qps/driver.cc index b745f741c49..ae13eb02074 100644 --- a/test/cpp/qps/driver.cc +++ b/test/cpp/qps/driver.cc @@ -533,7 +533,7 @@ std::unique_ptr RunScenario( rrc->set_count(it->second); } - // Collect servers' final run results right after finishing server + // Collect servers' final run results right after finishing server for (size_t i = 0; i < num_servers; i++) { auto server = &servers[i]; // Read the server final status