Make grpc_passthru_endpoint_stats refcounted by jtattermusch · Pull Request #14236 · grpc/grpc

jtattermusch · 2018-01-30T18:26:58Z

Attempt to fix #13445.

Still needs some cleanup.

jtattermusch · 2018-01-30T18:28:39Z

test/cpp/microbenchmarks/bm_fullstack_trickle.cc

  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


I'm not really proud of the way stats_ gets set as a side effect of MakeEndpoints, but I wanted to try if this fixes the flake first. Any ideas from a C++ expert here?

I'll send a sub-PR, but I'm speculating that you want something based on shared_ptr, make_shared, shared_from_this, etc

As discussed offline, I didn't use shared_ptr as passthru_endpoint.cc is under test/core and other files there doesn't use shared_ptr there yet either.

Can I suggest doing it in the constructor instead of in MakeEndpoints? This basically should only be created once either way, and it makes sense to create it in the constructor since it gets destroyed in the destructor.

Not sure I understand what you mean.
I need to pass MakeEndpoints to the base class constructor and in the body of this class' constructor it's too late for doing that.

grpc_endpoint_pair is an "abstract" struct, so later extracting the stats from it (after EndpointPairFixture is constructed and knowing it's an passthru_endpoint) wouldn't work either.

I think I solved it. PTAL.

grpc-testing · 2018-01-30T18:30:03Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-01-30T18:41:41Z

[trickle] No significant performance differences

jtattermusch · 2018-01-30T18:55:19Z

test/core/util/passthru_endpoint.h

@@ -24,6 +24,7 @@
 #include "src/core/lib/iomgr/endpoint.h"

 typedef struct {


I did update all the places where grpc_passthru_endpoint_stats is used to use a pointer, but in theory nothing is preventing the struct to be used as a by-value field grpc_passthru_endpoint_stats stats_ in new code (which is presumably not good because of the refs). Perhaps a better encapsulation would be useful.

This is test/core/util, so a comment putting that restriction in place should be sufficient.

comment added.

grpc-testing · 2018-01-30T19:23:21Z

[microbenchmarks] No significant performance differences

jtattermusch · 2018-01-30T21:41:36Z

This seems to be actually fixing the issue, I ran 5000 iterations locally and could not reproduce (I was able to reproduce before).

vjpai · 2018-01-31T19:06:57Z

test/core/util/passthru_endpoint.h

@@ -24,6 +24,7 @@
 #include "src/core/lib/iomgr/endpoint.h"

 typedef struct {


This is test/core/util, so a comment putting that restriction in place should be sufficient.

vjpai · 2018-01-31T19:10:41Z

test/cpp/microbenchmarks/bm_fullstack_trickle.cc

  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


Can I suggest doing it in the constructor instead of in MakeEndpoints? This basically should only be created once either way, and it makes sense to create it in the constructor since it gets destroyed in the destructor.

grpc-testing · 2018-01-31T21:41:59Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

jtattermusch · 2018-01-31T21:42:20Z

I think I solved the problem with stats_ initialization and MakeEndpoints() being called before base constructor, I used a default parameter
grpc_passthru_endpoint_stats* stats = grpc_passthru_endpoint_stats_create() in the constructor, which I think is a reasonable approach and it's cleaner that what we had before. PTAL.

grpc-testing · 2018-01-31T21:53:34Z

[trickle] No significant performance differences

jtattermusch · 2018-01-31T22:25:28Z

Fixed the problem with forbidden default arguments. PTAL.

grpc-testing · 2018-01-31T22:26:31Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

vjpai · 2018-01-31T22:29:26Z

test/cpp/microbenchmarks/bm_fullstack_trickle.cc

  gpr_timespec start_ = gpr_now(GPR_CLOCK_MONOTONIC);

-  grpc_endpoint_pair MakeEndpoints(size_t kilobits) {
+  static grpc_endpoint_pair MakeEndpoints(size_t kilobits,


The static was a very nice touch to explicitly identify that this won't touch any member variables

grpc-testing · 2018-01-31T22:34:05Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-01-31T22:38:45Z

[trickle] No significant performance differences

grpc-testing · 2018-01-31T23:18:37Z

[microbenchmarks] No significant performance differences

make grpc_passthru_endpoint_stats refcounted

12ba4b1

jtattermusch requested a review from vjpai January 30, 2018 18:27

jtattermusch commented Jan 30, 2018

View reviewed changes

vjpai suggested changes Jan 31, 2018

View reviewed changes

jtattermusch added 2 commits January 31, 2018 21:25

add comment

382374e

avoid touching stats_ instance field before base constructor

18f2737

calling create in default param is against code style

889bb7f

vjpai approved these changes Jan 31, 2018

View reviewed changes

jtattermusch changed the title ~~WIP: Make grpc_passthru_endpoint_stats refcounted~~ Make grpc_passthru_endpoint_stats refcounted Jan 31, 2018

jtattermusch merged commit e294279 into grpc:master Jan 31, 2018

lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019

		@@ -24,6 +24,7 @@
		#include "src/core/lib/iomgr/endpoint.h"

		typedef struct {

Conversation

jtattermusch commented Jan 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grpc-testing commented Jan 30, 2018

Uh oh!

grpc-testing commented Jan 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grpc-testing commented Jan 30, 2018

Uh oh!

jtattermusch commented Jan 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grpc-testing commented Jan 31, 2018

Uh oh!

jtattermusch commented Jan 31, 2018

Uh oh!

grpc-testing commented Jan 31, 2018

Uh oh!

jtattermusch commented Jan 31, 2018

Uh oh!

grpc-testing commented Jan 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grpc-testing commented Jan 31, 2018

Uh oh!

grpc-testing commented Jan 31, 2018

Uh oh!

grpc-testing commented Jan 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants