From b1db869e1a792c39a060ccca60983bcbddd9290c Mon Sep 17 00:00:00 2001 From: vjpai Date: Tue, 11 Aug 2015 22:41:02 -0700 Subject: [PATCH] Address concerns from review --- test/cpp/qps/client.h | 7 +++---- test/cpp/qps/driver.cc | 8 +++++--- test/cpp/qps/driver.h | 6 +++--- test/cpp/qps/interarrival.h | 2 +- test/cpp/qps/qps_driver.cc | 2 +- test/cpp/qps/report.cc | 8 ++++---- test/cpp/qps/server_async.cc | 2 +- 7 files changed, 18 insertions(+), 17 deletions(-) diff --git a/test/cpp/qps/client.h b/test/cpp/qps/client.h index a6bd1e43431..3d277d9e8ff 100644 --- a/test/cpp/qps/client.h +++ b/test/cpp/qps/client.h @@ -42,7 +42,6 @@ #include #include #include -#include namespace grpc { @@ -83,7 +82,7 @@ class Client { ClientStats Mark() { Histogram latencies; - // avoid std::vector for old compilers + // avoid std::vector for old compilers that expect a copy constructor Histogram *to_merge = new Histogram[threads_.size()]; for (size_t i = 0; i < threads_.size(); i++) { threads_[i]->BeginSwap(&to_merge[i]); @@ -113,7 +112,7 @@ class Client { class ClientChannelInfo { public: ClientChannelInfo() {} - ClientChannelInfo(const ClientChannelInfo& i) : channel_(), stub_() { + ClientChannelInfo(const ClientChannelInfo& i) { // The copy constructor is to satisfy old compilers // that need it for using std::vector . It is only ever // used for empty entries @@ -237,7 +236,7 @@ class Client { void ThreadFunc() { for (;;) { // run the loop body - bool thread_still_ok = client_->ThreadFunc(&histogram_, idx_); + const bool thread_still_ok = client_->ThreadFunc(&histogram_, idx_); // lock, see if we're done std::lock_guard g(mu_); if (!thread_still_ok) { diff --git a/test/cpp/qps/driver.cc b/test/cpp/qps/driver.cc index 71b0103fc77..4ef2d05dcd4 100644 --- a/test/cpp/qps/driver.cc +++ b/test/cpp/qps/driver.cc @@ -97,7 +97,7 @@ struct ClientData { unique_ptr stub; unique_ptr> stream; }; -} +} // namespace runsc std::unique_ptr RunScenario( const ClientConfig& initial_client_config, size_t num_clients, @@ -152,7 +152,7 @@ std::unique_ptr RunScenario( using runsc::ServerData; // servers is array rather than std::vector to avoid gcc-4.4 issues // where class contained in std::vector must have a copy constructor - ServerData servers[num_servers]; + auto* servers = new ServerData[num_servers]; for (size_t i = 0; i < num_servers; i++) { servers[i].stub = std::move(Worker::NewStub( CreateChannel(workers[i], InsecureCredentials(), ChannelArguments()))); @@ -180,7 +180,7 @@ std::unique_ptr RunScenario( using runsc::ClientData; // clients is array rather than std::vector to avoid gcc-4.4 issues // where class contained in std::vector must have a copy constructor - ClientData clients[num_clients]; + auto* clients = new ClientData[num_clients]; for (size_t i = 0; i < num_clients; i++) { clients[i].stub = std::move(Worker::NewStub(CreateChannel( workers[i + num_servers], InsecureCredentials(), ChannelArguments()))); @@ -262,6 +262,8 @@ std::unique_ptr RunScenario( GPR_ASSERT(server->stream->WritesDone()); GPR_ASSERT(server->stream->Finish().ok()); } + delete[] clients; + delete[] servers; return result; } } // namespace testing diff --git a/test/cpp/qps/driver.h b/test/cpp/qps/driver.h index 36a1e9d765c..9a29df8d494 100644 --- a/test/cpp/qps/driver.h +++ b/test/cpp/qps/driver.h @@ -45,9 +45,9 @@ class ResourceUsage { public: ResourceUsage(double w, double u, double s) : wall_time_(w), user_time_(u), system_time_(s) {} - double wall_time() { return wall_time_; } - double user_time() { return user_time_; } - double system_time() { return system_time_; } + double wall_time() const { return wall_time_; } + double user_time() const { return user_time_; } + double system_time() const { return system_time_; } private: double wall_time_; diff --git a/test/cpp/qps/interarrival.h b/test/cpp/qps/interarrival.h index da16a1553ff..04d14f689fc 100644 --- a/test/cpp/qps/interarrival.h +++ b/test/cpp/qps/interarrival.h @@ -149,7 +149,7 @@ class InterarrivalTimer { for (int i = 0; i < entries; i++) { // rand is the only choice that is portable across POSIX and Windows // and that supports new and old compilers - double uniform_0_1 = rand() / RAND_MAX; + const double uniform_0_1 = rand() / RAND_MAX; random_table_.push_back( std::chrono::nanoseconds(static_cast(1e9 * r(uniform_0_1)))); } diff --git a/test/cpp/qps/qps_driver.cc b/test/cpp/qps/qps_driver.cc index 7c85f81b5e8..b1463be8f62 100644 --- a/test/cpp/qps/qps_driver.cc +++ b/test/cpp/qps/qps_driver.cc @@ -33,10 +33,10 @@ #include #include +#include #include #include -#include #include "test/cpp/qps/driver.h" #include "test/cpp/qps/report.h" diff --git a/test/cpp/qps/report.cc b/test/cpp/qps/report.cc index 884e15690d1..d89a286ca05 100644 --- a/test/cpp/qps/report.cc +++ b/test/cpp/qps/report.cc @@ -140,13 +140,13 @@ void PerfDbReporter::ReportLatency(const ScenarioResult& result) { } void PerfDbReporter::ReportTimes(const ScenarioResult& result) { - double server_system_time = 100.0 * sum(result.server_resources, SystemTime) / + const double server_system_time = 100.0 * sum(result.server_resources, SystemTime) / sum(result.server_resources, WallTime); - double server_user_time = 100.0 * sum(result.server_resources, UserTime) / + const double server_user_time = 100.0 * sum(result.server_resources, UserTime) / sum(result.server_resources, WallTime); - double client_system_time = 100.0 * sum(result.client_resources, SystemTime) / + const double client_system_time = 100.0 * sum(result.client_resources, SystemTime) / sum(result.client_resources, WallTime); - double client_user_time = 100.0 * sum(result.client_resources, UserTime) / + const double client_user_time = 100.0 * sum(result.client_resources, UserTime) / sum(result.client_resources, WallTime); perf_db_client_.setTimes(server_system_time, server_user_time, diff --git a/test/cpp/qps/server_async.cc b/test/cpp/qps/server_async.cc index 41e873c3856..b4fc49c31c3 100644 --- a/test/cpp/qps/server_async.cc +++ b/test/cpp/qps/server_async.cc @@ -131,7 +131,7 @@ class AsyncQpsServerTest : public Server { while (srv_cqs_[rank]->Next(&got_tag, &ok)) { ServerRpcContext *ctx = detag(got_tag); // The tag is a pointer to an RPC context to invoke - bool still_going = ctx->RunNextState(ok); + const bool still_going = ctx->RunNextState(ok); if (!shutdown_state_[rank]->shutdown()) { // this RPC context is done, so refresh it if (!still_going) {