Implement InlinedVector independently of absl. by markdroth · Pull Request #13676 · grpc/grpc

markdroth · 2017-12-07T18:17:35Z

Splitting this off from the retry PR, since I'm going to want to use it in other code as well.

grpc-testing · 2017-12-07T18:21:15Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2017-12-07T18:33:46Z

[trickle] No significant performance differences

grpc-testing · 2017-12-07T18:55:26Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2017-12-07T19:07:59Z

[trickle] No significant performance differences

grpc-testing · 2017-12-07T19:13:01Z

[microbenchmarks] No significant performance differences

grpc-testing · 2017-12-07T19:48:44Z

[microbenchmarks] No significant performance differences

grpc-testing · 2017-12-07T20:51:39Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2017-12-07T21:02:08Z

[trickle] No significant performance differences

grpc-testing · 2017-12-07T21:44:00Z

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_unary_ping_pong.BM_UnaryPingPong_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__64_0.opt.new: 1


[microbenchmarks] No significant performance differences

grpc-testing · 2017-12-08T17:57:15Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2017-12-08T18:10:06Z

[trickle] No significant performance differences

grpc-testing · 2017-12-08T18:48:38Z

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_unary_ping_pong.BM_UnaryPingPong_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__512_512.opt.old: 1


[microbenchmarks] No significant performance differences

grpc-testing · 2017-12-12T18:27:37Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2017-12-12T18:39:19Z

[trickle] No significant performance differences

grpc-testing · 2017-12-12T19:19:02Z

[microbenchmarks] No significant performance differences

dgquintas · 2018-01-09T22:41:40Z

build.yaml

  - grpc
  - gpr_test_util
  - gpr
+- name: vector_test


you should also update the corresponding BUILD file

It's already there. See test/core/support/BUILD.

dgquintas · 2018-01-09T22:55:38Z

src/core/lib/support/vector.h

+  template <typename... Args>
+  void emplace_back(Args&&... args) {
+    if (size_ < N) {
+      new (&inline_[size_]) T(std::forward<Args>(args)...);


We can't use new. Prefer this. Idem for line 85 and 92 below.

This is placement-new, not allocate-new. It's the same syntax used in the New() function you pointed at.

dgquintas · 2018-01-09T22:56:39Z

src/core/lib/support/vector.h

+  InlinedVector& operator=(const InlinedVector&) = delete;
+
+  T& operator[](size_t offset) {
+    assert(offset < size_);


why not GPR_ASSERT?

That's what I had originally, but @ctiller said to switch to this.

Craig, was this because this is more idiomatic in C++?

dgquintas · 2018-01-09T23:00:16Z

test/core/support/vector_test.cc

+  }
+}
+
+TEST(InlinedVectorTest, ValuesAreInlined) {


would it be possible to test whether the values really are inlined, as opposed to placed in heap memory?

I'm not sure I see an easy way to do that. Any suggestions?

There are ways of detecting whether a variable lives on the stack vs the heap, but unfortunately they aren't portable. And for a tiny test like this it's not worth doing more sophisticated stuff such as using custom allocators. Nevermind.

dgquintas · 2018-01-09T23:01:30Z

test/core/support/vector_test.cc

+TEST(InlinedVectorTest, PushBackWithMove) {
+  InlinedVector<UniquePtr<int>, 1> v;
+  UniquePtr<int> i = MakeUnique<int>(3);
+  v.push_back(std::move(i));


we could check that i has been null'ed after the move.

grpc-testing · 2018-01-10T15:51:31Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-01-10T16:02:30Z

[trickle] No significant performance differences

grpc-testing · 2018-01-10T16:44:22Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-01-10T17:11:41Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-01-10T17:24:49Z

[trickle] No significant performance differences

grpc-testing · 2018-01-10T18:04:26Z

[microbenchmarks] No significant performance differences

dgquintas · 2018-01-10T18:04:51Z

test/core/support/vector_test.cc

+  }
+}
+
+TEST(InlinedVectorTest, ValuesAreInlined) {


There are ways of detecting whether a variable lives on the stack vs the heap, but unfortunately they aren't portable. And for a tiny test like this it's not worth doing more sophisticated stuff such as using custom allocators. Nevermind.

markdroth · 2018-01-10T22:44:12Z

Known failures: #10092 #13973

grpc-testing · 2018-01-10T22:51:01Z

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-01-10T23:02:25Z

[trickle] No significant performance differences

grpc-testing · 2018-01-10T23:42:42Z

[microbenchmarks] No significant performance differences

markdroth · 2018-01-11T15:00:57Z

Known issue: #13953

markdroth requested a review from ctiller December 7, 2017 18:17

markdroth mentioned this pull request Dec 7, 2017

C++ Resolver API #13684

Merged

markdroth requested review from dgquintas and removed request for ctiller January 8, 2018 21:59

dgquintas suggested changes Jan 9, 2018

View reviewed changes

dgquintas approved these changes Jan 10, 2018

View reviewed changes

Implement InlinedVector independently of absl.

c6406f3

markdroth force-pushed the inlined_vector branch from d601da1 to c6406f3 Compare January 10, 2018 22:47

markdroth merged commit 5343053 into grpc:master Jan 11, 2018

markdroth deleted the inlined_vector branch January 11, 2018 15:01

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

Conversation

markdroth commented Dec 7, 2017

Uh oh!

grpc-testing commented Dec 7, 2017

Uh oh!

grpc-testing commented Dec 7, 2017

Uh oh!

grpc-testing commented Dec 7, 2017

Uh oh!

grpc-testing commented Dec 7, 2017

Uh oh!

grpc-testing commented Dec 7, 2017

Uh oh!

grpc-testing commented Dec 7, 2017

Uh oh!

grpc-testing commented Dec 7, 2017

Uh oh!

grpc-testing commented Dec 7, 2017

Uh oh!

grpc-testing commented Dec 7, 2017

Uh oh!

grpc-testing commented Dec 8, 2017

Uh oh!

grpc-testing commented Dec 8, 2017

Uh oh!

grpc-testing commented Dec 8, 2017

Uh oh!

grpc-testing commented Dec 12, 2017

Uh oh!

grpc-testing commented Dec 12, 2017

Uh oh!

grpc-testing commented Dec 12, 2017

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!

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 10, 2018

Uh oh!

grpc-testing commented Jan 10, 2018

Uh oh!

grpc-testing commented Jan 10, 2018

Uh oh!

grpc-testing commented Jan 10, 2018

Uh oh!

grpc-testing commented Jan 10, 2018

Uh oh!

grpc-testing commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markdroth commented Jan 10, 2018

Uh oh!

grpc-testing commented Jan 10, 2018

Uh oh!

grpc-testing commented Jan 10, 2018

Uh oh!

grpc-testing commented Jan 10, 2018

Uh oh!

markdroth commented Jan 11, 2018

Uh oh!

Reviewers