From 12ba4b1e05ca420008c38234e13c5540b3b704c0 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Tue, 30 Jan 2018 19:12:59 +0100 Subject: make grpc_passthru_endpoint_stats refcounted --- test/core/util/passthru_endpoint.cc | 26 ++++++++++++++++++++---- test/core/util/passthru_endpoint.h | 5 +++++ test/cpp/microbenchmarks/bm_fullstack_trickle.cc | 13 +++++++++--- test/cpp/microbenchmarks/fullstack_fixtures.h | 13 +++++++++--- test/cpp/performance/writes_per_rpc_test.cc | 13 +++++++++--- 5 files changed, 57 insertions(+), 13 deletions(-) (limited to 'test') diff --git a/test/core/util/passthru_endpoint.cc b/test/core/util/passthru_endpoint.cc index 5f127cb960..0da0765979 100644 --- a/test/core/util/passthru_endpoint.cc +++ b/test/core/util/passthru_endpoint.cc @@ -48,8 +48,6 @@ struct passthru_endpoint { gpr_mu mu; int halves; grpc_passthru_endpoint_stats* stats; - grpc_passthru_endpoint_stats - dummy_stats; // used if constructor stats == nullptr bool shutdown; half client; half server; @@ -137,6 +135,7 @@ static void me_destroy(grpc_endpoint* ep) { if (0 == --p->halves) { gpr_mu_unlock(&p->mu); gpr_mu_destroy(&p->mu); + grpc_passthru_endpoint_stats_destroy(p->stats); grpc_slice_buffer_destroy_internal(&p->client.read_buffer); grpc_slice_buffer_destroy_internal(&p->server.read_buffer); grpc_resource_user_unref(p->client.resource_user); @@ -194,11 +193,30 @@ void grpc_passthru_endpoint_create(grpc_endpoint** client, passthru_endpoint* m = (passthru_endpoint*)gpr_malloc(sizeof(*m)); m->halves = 2; m->shutdown = 0; - m->stats = stats == nullptr ? &m->dummy_stats : stats; - memset(m->stats, 0, sizeof(*m->stats)); + if (stats == nullptr) { + m->stats = grpc_passthru_endpoint_stats_create(); + } else { + gpr_ref(&stats->refs); + m->stats = stats; + } half_init(&m->client, m, resource_quota, "client"); half_init(&m->server, m, resource_quota, "server"); gpr_mu_init(&m->mu); *client = &m->client.base; *server = &m->server.base; } + +grpc_passthru_endpoint_stats* grpc_passthru_endpoint_stats_create() { + grpc_passthru_endpoint_stats* stats = + (grpc_passthru_endpoint_stats*)gpr_malloc( + sizeof(grpc_passthru_endpoint_stats)); + memset(stats, 0, sizeof(*stats)); + gpr_ref_init(&stats->refs, 1); + return stats; +} + +void grpc_passthru_endpoint_stats_destroy(grpc_passthru_endpoint_stats* stats) { + if (gpr_unref(&stats->refs)) { + gpr_free(stats); + } +} diff --git a/test/core/util/passthru_endpoint.h b/test/core/util/passthru_endpoint.h index bddd8ea6a2..355b244688 100644 --- a/test/core/util/passthru_endpoint.h +++ b/test/core/util/passthru_endpoint.h @@ -24,6 +24,7 @@ #include "src/core/lib/iomgr/endpoint.h" typedef struct { + gpr_refcount refs; gpr_atm num_writes; } grpc_passthru_endpoint_stats; @@ -32,4 +33,8 @@ void grpc_passthru_endpoint_create(grpc_endpoint** client, grpc_resource_quota* resource_quota, grpc_passthru_endpoint_stats* stats); +grpc_passthru_endpoint_stats* grpc_passthru_endpoint_stats_create(); + +void grpc_passthru_endpoint_stats_destroy(grpc_passthru_endpoint_stats* stats); + #endif diff --git a/test/cpp/microbenchmarks/bm_fullstack_trickle.cc b/test/cpp/microbenchmarks/bm_fullstack_trickle.cc index d6d7d41e5e..07a8102915 100644 --- a/test/cpp/microbenchmarks/bm_fullstack_trickle.cc +++ b/test/cpp/microbenchmarks/bm_fullstack_trickle.cc @@ -101,9 +101,11 @@ class TrickledCHTTP2 : public EndpointPairFixture { } } + virtual ~TrickledCHTTP2() { grpc_passthru_endpoint_stats_destroy(stats_); } + void AddToLabel(std::ostream& out, benchmark::State& state) { out << " writes/iter:" - << ((double)stats_.num_writes / (double)state.iterations()) + << ((double)stats_->num_writes / (double)state.iterations()) << " cli_transport_stalls/iter:" << ((double) client_stats_.streams_stalled_due_to_transport_flow_control / @@ -193,7 +195,7 @@ class TrickledCHTTP2 : public EndpointPairFixture { } private: - grpc_passthru_endpoint_stats stats_; + grpc_passthru_endpoint_stats* stats_; struct Stats { int streams_stalled_due_to_stream_flow_control = 0; int streams_stalled_due_to_transport_flow_control = 0; @@ -204,9 +206,14 @@ class TrickledCHTTP2 : public EndpointPairFixture { gpr_timespec start_ = gpr_now(GPR_CLOCK_MONOTONIC); grpc_endpoint_pair MakeEndpoints(size_t kilobits) { + stats_ = grpc_passthru_endpoint_stats_create(); // is there a better way to + // initialize stats_ and + // pass MakeEndpoints's + // return value to base + // constructor? grpc_endpoint_pair p; grpc_passthru_endpoint_create(&p.client, &p.server, Library::get().rq(), - &stats_); + stats_); double bytes_per_second = 125.0 * kilobits; p.client = grpc_trickle_endpoint_create(p.client, bytes_per_second); p.server = grpc_trickle_endpoint_create(p.server, bytes_per_second); diff --git a/test/cpp/microbenchmarks/fullstack_fixtures.h b/test/cpp/microbenchmarks/fullstack_fixtures.h index d1ede755a5..7e93cbd74d 100644 --- a/test/cpp/microbenchmarks/fullstack_fixtures.h +++ b/test/cpp/microbenchmarks/fullstack_fixtures.h @@ -252,20 +252,27 @@ class InProcessCHTTP2 : public EndpointPairFixture { FixtureConfiguration()) : EndpointPairFixture(service, MakeEndpoints(), fixture_configuration) {} + virtual ~InProcessCHTTP2() { grpc_passthru_endpoint_stats_destroy(stats_); } + void AddToLabel(std::ostream& out, benchmark::State& state) { EndpointPairFixture::AddToLabel(out, state); out << " writes/iter:" - << static_cast(gpr_atm_no_barrier_load(&stats_.num_writes)) / + << static_cast(gpr_atm_no_barrier_load(&stats_->num_writes)) / static_cast(state.iterations()); } private: - grpc_passthru_endpoint_stats stats_; + grpc_passthru_endpoint_stats* stats_; grpc_endpoint_pair MakeEndpoints() { + stats_ = grpc_passthru_endpoint_stats_create(); // is there a better way to + // initialize stats_ and + // pass MakeEndpoints's + // return value to base + // constructor? grpc_endpoint_pair p; grpc_passthru_endpoint_create(&p.client, &p.server, Library::get().rq(), - &stats_); + stats_); return p; } }; diff --git a/test/cpp/performance/writes_per_rpc_test.cc b/test/cpp/performance/writes_per_rpc_test.cc index 0b9dc83f2b..0866b58f58 100644 --- a/test/cpp/performance/writes_per_rpc_test.cc +++ b/test/cpp/performance/writes_per_rpc_test.cc @@ -145,15 +145,22 @@ class InProcessCHTTP2 : public EndpointPairFixture { InProcessCHTTP2(Service* service) : EndpointPairFixture(service, MakeEndpoints()) {} - int writes_performed() const { return stats_.num_writes; } + virtual ~InProcessCHTTP2() { grpc_passthru_endpoint_stats_destroy(stats_); } + + int writes_performed() const { return stats_->num_writes; } private: - grpc_passthru_endpoint_stats stats_; + grpc_passthru_endpoint_stats* stats_; grpc_endpoint_pair MakeEndpoints() { + stats_ = grpc_passthru_endpoint_stats_create(); // is there a better way to + // initialize stats_ and + // pass MakeEndpoints's + // return value to base + // constructor? grpc_endpoint_pair p; grpc_passthru_endpoint_create(&p.client, &p.server, initialize_stuff.rq(), - &stats_); + stats_); return p; } }; -- cgit v1.2.3 From 382374eb6f373cb07b9b11371d6dda43138f0c26 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Wed, 31 Jan 2018 21:25:30 +0100 Subject: add comment --- test/core/util/passthru_endpoint.h | 3 +++ 1 file changed, 3 insertions(+) (limited to 'test') diff --git a/test/core/util/passthru_endpoint.h b/test/core/util/passthru_endpoint.h index 355b244688..a46c775505 100644 --- a/test/core/util/passthru_endpoint.h +++ b/test/core/util/passthru_endpoint.h @@ -23,6 +23,9 @@ #include "src/core/lib/iomgr/endpoint.h" +/* The struct is refcounted, always use grpc_passthru_endpoint_stats_create and + * grpc_passthru_endpoint_stats_destroy, rather then embedding it in your + * objects by value. */ typedef struct { gpr_refcount refs; gpr_atm num_writes; -- cgit v1.2.3 From 18f27376c531e857d8fb946d758c81880023ed42 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Wed, 31 Jan 2018 22:35:17 +0100 Subject: avoid touching stats_ instance field before base constructor --- test/cpp/microbenchmarks/bm_fullstack_trickle.cc | 25 +++++++++++++----------- test/cpp/microbenchmarks/fullstack_fixtures.h | 25 +++++++++++++----------- test/cpp/performance/writes_per_rpc_test.cc | 20 +++++++++---------- 3 files changed, 38 insertions(+), 32 deletions(-) (limited to 'test') diff --git a/test/cpp/microbenchmarks/bm_fullstack_trickle.cc b/test/cpp/microbenchmarks/bm_fullstack_trickle.cc index 07a8102915..3e31c2a4cb 100644 --- a/test/cpp/microbenchmarks/bm_fullstack_trickle.cc +++ b/test/cpp/microbenchmarks/bm_fullstack_trickle.cc @@ -79,9 +79,12 @@ static void write_csv(std::ostream* out, A0&& a0, Arg&&... arg) { class TrickledCHTTP2 : public EndpointPairFixture { public: TrickledCHTTP2(Service* service, bool streaming, size_t req_size, - size_t resp_size, size_t kilobits_per_second) - : EndpointPairFixture(service, MakeEndpoints(kilobits_per_second), - FixtureConfiguration()) { + size_t resp_size, size_t kilobits_per_second, + grpc_passthru_endpoint_stats* stats = + grpc_passthru_endpoint_stats_create()) + : EndpointPairFixture(service, MakeEndpoints(kilobits_per_second, stats), + FixtureConfiguration()), + stats_(stats) { if (FLAGS_log) { std::ostringstream fn; fn << "trickle." << (streaming ? "streaming" : "unary") << "." << req_size @@ -101,7 +104,11 @@ class TrickledCHTTP2 : public EndpointPairFixture { } } - virtual ~TrickledCHTTP2() { grpc_passthru_endpoint_stats_destroy(stats_); } + virtual ~TrickledCHTTP2() { + if (stats_ != nullptr) { + grpc_passthru_endpoint_stats_destroy(stats_); + } + } void AddToLabel(std::ostream& out, benchmark::State& state) { out << " writes/iter:" @@ -205,15 +212,11 @@ class TrickledCHTTP2 : public EndpointPairFixture { std::unique_ptr log_; gpr_timespec start_ = gpr_now(GPR_CLOCK_MONOTONIC); - grpc_endpoint_pair MakeEndpoints(size_t kilobits) { - stats_ = grpc_passthru_endpoint_stats_create(); // is there a better way to - // initialize stats_ and - // pass MakeEndpoints's - // return value to base - // constructor? + static grpc_endpoint_pair MakeEndpoints(size_t kilobits, + grpc_passthru_endpoint_stats* stats) { grpc_endpoint_pair p; grpc_passthru_endpoint_create(&p.client, &p.server, Library::get().rq(), - stats_); + stats); double bytes_per_second = 125.0 * kilobits; p.client = grpc_trickle_endpoint_create(p.client, bytes_per_second); p.server = grpc_trickle_endpoint_create(p.server, bytes_per_second); diff --git a/test/cpp/microbenchmarks/fullstack_fixtures.h b/test/cpp/microbenchmarks/fullstack_fixtures.h index 7e93cbd74d..00ec72deee 100644 --- a/test/cpp/microbenchmarks/fullstack_fixtures.h +++ b/test/cpp/microbenchmarks/fullstack_fixtures.h @@ -249,10 +249,18 @@ class InProcessCHTTP2 : public EndpointPairFixture { public: InProcessCHTTP2(Service* service, const FixtureConfiguration& fixture_configuration = - FixtureConfiguration()) - : EndpointPairFixture(service, MakeEndpoints(), fixture_configuration) {} - - virtual ~InProcessCHTTP2() { grpc_passthru_endpoint_stats_destroy(stats_); } + FixtureConfiguration(), + grpc_passthru_endpoint_stats* stats = + grpc_passthru_endpoint_stats_create()) + : EndpointPairFixture(service, MakeEndpoints(stats), + fixture_configuration), + stats_(stats) {} + + virtual ~InProcessCHTTP2() { + if (stats_ != nullptr) { + grpc_passthru_endpoint_stats_destroy(stats_); + } + } void AddToLabel(std::ostream& out, benchmark::State& state) { EndpointPairFixture::AddToLabel(out, state); @@ -264,15 +272,10 @@ class InProcessCHTTP2 : public EndpointPairFixture { private: grpc_passthru_endpoint_stats* stats_; - grpc_endpoint_pair MakeEndpoints() { - stats_ = grpc_passthru_endpoint_stats_create(); // is there a better way to - // initialize stats_ and - // pass MakeEndpoints's - // return value to base - // constructor? + static grpc_endpoint_pair MakeEndpoints(grpc_passthru_endpoint_stats* stats) { grpc_endpoint_pair p; grpc_passthru_endpoint_create(&p.client, &p.server, Library::get().rq(), - stats_); + stats); return p; } }; diff --git a/test/cpp/performance/writes_per_rpc_test.cc b/test/cpp/performance/writes_per_rpc_test.cc index 0866b58f58..86e3bbcd4e 100644 --- a/test/cpp/performance/writes_per_rpc_test.cc +++ b/test/cpp/performance/writes_per_rpc_test.cc @@ -142,25 +142,25 @@ class EndpointPairFixture { class InProcessCHTTP2 : public EndpointPairFixture { public: - InProcessCHTTP2(Service* service) - : EndpointPairFixture(service, MakeEndpoints()) {} + InProcessCHTTP2(Service* service, grpc_passthru_endpoint_stats* stats = + grpc_passthru_endpoint_stats_create()) + : EndpointPairFixture(service, MakeEndpoints(stats)), stats_(stats) {} - virtual ~InProcessCHTTP2() { grpc_passthru_endpoint_stats_destroy(stats_); } + virtual ~InProcessCHTTP2() { + if (stats_ != nullptr) { + grpc_passthru_endpoint_stats_destroy(stats_); + } + } int writes_performed() const { return stats_->num_writes; } private: grpc_passthru_endpoint_stats* stats_; - grpc_endpoint_pair MakeEndpoints() { - stats_ = grpc_passthru_endpoint_stats_create(); // is there a better way to - // initialize stats_ and - // pass MakeEndpoints's - // return value to base - // constructor? + static grpc_endpoint_pair MakeEndpoints(grpc_passthru_endpoint_stats* stats) { grpc_endpoint_pair p; grpc_passthru_endpoint_create(&p.client, &p.server, initialize_stuff.rq(), - stats_); + stats); return p; } }; -- cgit v1.2.3 From 889bb7fde4c3f8f36c3821e16b0b66870e68526d Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Wed, 31 Jan 2018 23:21:41 +0100 Subject: calling create in default param is against code style --- test/cpp/microbenchmarks/bm_fullstack_trickle.cc | 9 ++++---- test/cpp/microbenchmarks/fullstack_fixtures.h | 26 +++++++++++++++++------- test/cpp/performance/writes_per_rpc_test.cc | 6 +++--- 3 files changed, 27 insertions(+), 14 deletions(-) (limited to 'test') diff --git a/test/cpp/microbenchmarks/bm_fullstack_trickle.cc b/test/cpp/microbenchmarks/bm_fullstack_trickle.cc index 3e31c2a4cb..294f1feb80 100644 --- a/test/cpp/microbenchmarks/bm_fullstack_trickle.cc +++ b/test/cpp/microbenchmarks/bm_fullstack_trickle.cc @@ -80,8 +80,7 @@ class TrickledCHTTP2 : public EndpointPairFixture { public: TrickledCHTTP2(Service* service, bool streaming, size_t req_size, size_t resp_size, size_t kilobits_per_second, - grpc_passthru_endpoint_stats* stats = - grpc_passthru_endpoint_stats_create()) + grpc_passthru_endpoint_stats* stats) : EndpointPairFixture(service, MakeEndpoints(kilobits_per_second, stats), FixtureConfiguration()), stats_(stats) { @@ -261,7 +260,8 @@ static void BM_PumpStreamServerToClient_Trickle(benchmark::State& state) { EchoTestService::AsyncService service; std::unique_ptr fixture(new TrickledCHTTP2( &service, true, state.range(0) /* req_size */, - state.range(0) /* resp_size */, state.range(1) /* bw in kbit/s */)); + state.range(0) /* resp_size */, state.range(1) /* bw in kbit/s */, + grpc_passthru_endpoint_stats_create())); { EchoResponse send_response; EchoResponse recv_response; @@ -354,7 +354,8 @@ static void BM_PumpUnbalancedUnary_Trickle(benchmark::State& state) { EchoTestService::AsyncService service; std::unique_ptr fixture(new TrickledCHTTP2( &service, false, state.range(0) /* req_size */, - state.range(1) /* resp_size */, state.range(2) /* bw in kbit/s */)); + state.range(1) /* resp_size */, state.range(2) /* bw in kbit/s */, + grpc_passthru_endpoint_stats_create())); EchoRequest send_request; EchoResponse send_response; EchoResponse recv_response; diff --git a/test/cpp/microbenchmarks/fullstack_fixtures.h b/test/cpp/microbenchmarks/fullstack_fixtures.h index 00ec72deee..d73caa01c8 100644 --- a/test/cpp/microbenchmarks/fullstack_fixtures.h +++ b/test/cpp/microbenchmarks/fullstack_fixtures.h @@ -245,18 +245,20 @@ class SockPair : public EndpointPairFixture { fixture_configuration) {} }; -class InProcessCHTTP2 : public EndpointPairFixture { +/* Use InProcessCHTTP2 instead. This class (with stats as an explicit parameter) + is here only to be able to initialize both the base class and stats_ with the + same stats instance without accessing the stats_ fields before the object is + properly initialized. */ +class InProcessCHTTP2WithExplicitStats : public EndpointPairFixture { public: - InProcessCHTTP2(Service* service, - const FixtureConfiguration& fixture_configuration = - FixtureConfiguration(), - grpc_passthru_endpoint_stats* stats = - grpc_passthru_endpoint_stats_create()) + InProcessCHTTP2WithExplicitStats( + Service* service, grpc_passthru_endpoint_stats* stats, + const FixtureConfiguration& fixture_configuration) : EndpointPairFixture(service, MakeEndpoints(stats), fixture_configuration), stats_(stats) {} - virtual ~InProcessCHTTP2() { + virtual ~InProcessCHTTP2WithExplicitStats() { if (stats_ != nullptr) { grpc_passthru_endpoint_stats_destroy(stats_); } @@ -280,6 +282,16 @@ class InProcessCHTTP2 : public EndpointPairFixture { } }; +class InProcessCHTTP2 : public InProcessCHTTP2WithExplicitStats { + public: + InProcessCHTTP2(Service* service, + const FixtureConfiguration& fixture_configuration = + FixtureConfiguration()) + : InProcessCHTTP2WithExplicitStats(service, + grpc_passthru_endpoint_stats_create(), + fixture_configuration) {} +}; + //////////////////////////////////////////////////////////////////////////////// // Minimal stack fixtures diff --git a/test/cpp/performance/writes_per_rpc_test.cc b/test/cpp/performance/writes_per_rpc_test.cc index 86e3bbcd4e..b7d951a86e 100644 --- a/test/cpp/performance/writes_per_rpc_test.cc +++ b/test/cpp/performance/writes_per_rpc_test.cc @@ -142,8 +142,7 @@ class EndpointPairFixture { class InProcessCHTTP2 : public EndpointPairFixture { public: - InProcessCHTTP2(Service* service, grpc_passthru_endpoint_stats* stats = - grpc_passthru_endpoint_stats_create()) + InProcessCHTTP2(Service* service, grpc_passthru_endpoint_stats* stats) : EndpointPairFixture(service, MakeEndpoints(stats)), stats_(stats) {} virtual ~InProcessCHTTP2() { @@ -169,7 +168,8 @@ static double UnaryPingPong(int request_size, int response_size) { const int kIterations = 10000; EchoTestService::AsyncService service; - std::unique_ptr fixture(new InProcessCHTTP2(&service)); + std::unique_ptr fixture( + new InProcessCHTTP2(&service, grpc_passthru_endpoint_stats_create())); EchoRequest send_request; EchoResponse send_response; EchoResponse recv_response; -- cgit v1.2.3