quiche: implement QuicMemSlice by danzh2010 · Pull Request #6400 · envoyproxy/envoy

danzh2010 · 2019-03-27T17:46:59Z

Add QuicMemSliceImpl, QuicMemSliceSpanImpl, QuicMemSliceStorageImpl, QuicTestMemSliceVectorImpl using Envoy::Buffer;
Add several quic core targets into quiche.BUILD file to compile some quic objects including quic_types.h which is needed in QuicMemSliceSpan and quic_utils.h which is needed in QuicTestMemSliceVectorImpl;

Risk Level: low, not in use
Testing: tested impl's with quic_mem_slice_test.cc, quic_mem_slice_span_test.cc, quic_mem_slice_storage_test.cc from quiche.
Part of #2557

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-03-27T19:32:27Z

/assign @wu-bin

Signed-off-by: Dan Zhang <danzh@google.com>

wu-bin

I only finished the BUILD files, will continue on the c++ files.

wu-bin · 2019-03-28T16:27:10Z

+)
+
+envoy_cc_library(
+    name = "quic_mem_slice_storage_impl_lib",


nit: quic_platform_mem_slice_storage_impl_lib

htuch · 2019-03-29T14:58:45Z

I think @jmarantz is an ideal reviewer for this, as he has been doing other detailed reviews around buffer implementation recently.

Signed-off-by: Dan Zhang <danzh@google.com>

jmarantz

flushing some comments. I haven't deeply looked at the interaction with buffers yet.

jmarantz · 2019-03-29T15:06:08Z

+// NOLINT(namespace-envoy)
+
+// This file is part of the QUICHE platform implementation, and is not to be
+// consumed or referenced directly by other Envoy


unfinished sentence?

completed it.

jmarantz · 2019-03-29T15:11:03Z

+
+  // Constructs a QuicMemSliceImpl by let |allocator| allocate a data buffer of
+  // |length|.
+  QuicMemSliceImpl(QuicBufferAllocator* allocator, size_t length)


Shall we go with Envoy style guide and use non-const refs for pointers which we can't allow to be null?

Since all the impl's are used by quic API's, it inevitably has to match the expectation of its callers, and thus following quic style guide.

jmarantz · 2019-03-29T15:12:47Z

+        allocator->New(length), length,
+        [allocator](const void* data, size_t, const Envoy::Buffer::BufferFragmentImpl* fragment) {
+          allocator->Delete(const_cast<char*>(static_cast<const char*>(data)));
+          delete fragment;


i'm surprised this 'delete' compiles since it's passed as a const pointer.

deleter can still be called for const object so that it can be destructed.

jmarantz · 2019-03-29T15:34:43Z

+  QuicMemSliceStorageImpl(QuicMemSliceStorageImpl&& other) = default;
+  QuicMemSliceStorageImpl& operator=(QuicMemSliceStorageImpl&& other) = default;
+
+  ~QuicMemSliceStorageImpl() = default;


not needed.

jmarantz · 2019-03-29T15:40:54Z

+// consumed or referenced directly by other Envoy code. It serves purely as a
+// porting layer for QUICHE.
+
+#include <stdint.h>


#include <cstdint>

Signed-off-by: Dan Zhang <danzh@google.com>

wu-bin · 2019-03-28T18:04:51Z

+
+inline void QuicPrintCommandLineFlagHelpImpl(const char* /*usage*/) {}
+
+// porting layer for QUICHE.


Remove misplaced comment.

wu-bin · 2019-03-28T18:12:00Z

 #define QUIC_NOTREACHED_IMPL() NOT_REACHED_GCOVR_EXCL_LINE
 #endif

+#define DCHECK_GT(a, b) DCHECK(a > b)


Add parentheses to a and b to avoid problems with operator precedence:

#define DCHECK_GT(a, b) DCHECK((a) > (b))

Same below.

wu-bin · 2019-03-28T18:30:46Z

+  // Constructs a QuicMemSliceImpl by let |allocator| allocate a data buffer of
+  // |length|.
+  QuicMemSliceImpl(QuicBufferAllocator* allocator, size_t length)
+      : length_(length), slice_(new Envoy::Buffer::OwnedImpl()) {


This function seems too heavyweight, in theory it only needs to do one memory allocation(the lifetime of the memory is the same as QuicMemSliceImpl itself), but it currently does

2 allocations for slice_ (OwnedImpl + shared memory control block. Can change to std::make_shared to combine them)

1 allocation for fragment

1 allocation for the buffer itself (allocator->New)

1 allocation for the "UnownedSlice" object (inside the addBufferFragment call)

I think, without changing the overall approach in this PR, we can change the shared_ptr to a unique_ptr, which allows us to combine 1, 2 and 3 in one allocation(allocator->New(total size needed)). I suspect we'll want to change to approach in the future(i.e. replace Buffer instance in this class by a real slice object, which represents a continuous span of memory) to further optimize it, though.

I changed the class definition a bit:
Since mem slice doesn't need to share ownership of slice_ with other instance, slice_ doesn't need to be a shared_ptr.
Combined allocation of fragment and buffer itself.
Now there are only 2 allocations in this constructor.

wu-bin · 2019-03-29T16:47:03Z

+
+  // Constructs a QuicMemSliceImpl from a Buffer::Instance whose getRawSlices()
+  // returns only 1 slice.
+  QuicMemSliceImpl(std::shared_ptr<Envoy::Buffer::Instance> slice)


(If we change slice_ to unique_ptr) slice should also be a unique_ptr.

Should this constructor be 'explicit', since it only has 1 arg?

added explicit specifier.

wu-bin · 2019-03-29T16:59:30Z

+  bool empty() const { return length() == 0; }
+
+private:
+  size_t length_;


It seems length_ is not needed because slice_ already has a length() method.

wu-bin · 2019-03-29T17:05:49Z

+
+private:
+  size_t length_;
+  std::shared_ptr<Envoy::Buffer::Instance> slice_;


See my previous comment; I think this can be simplified to a unique_ptr.

Also a nit: The term "slice" often implies a continuous span of memory, I'd prefer something like 'single_slice_buffer_' here.

Rename to single_slice_buffer_.

wu-bin · 2019-03-29T17:49:15Z

+      // TODO(danzh): investigate the cost of allocating one buffer per slice.
+      // If it turns out to be expensive, add a new function to free data in the middle in buffer
+      // interface and re-design QuicMemSliceImpl.
+      single_slice_buffer->move(buffer_, slice.len_);


nit: DCHECK to make sure single_slice_buffer only has 1 slice?

There is ASSERT in QuicMemSliceImpl() one line below. Skipped this temporary variable here and passed in buffer_ directly.

wu-bin · 2019-03-29T18:02:13Z

+    void* dst = allocator->New(slice_len);
+    QuicUtils::CopyToBuffer(iov, iov_count, io_offset, slice_len, static_cast<char*>(dst));
+    io_offset += slice_len;
+    auto fragment = new Envoy::Buffer::BufferFragmentImpl(


Similar here, the allocation of data and BufferFragment can be combined.

Signed-off-by: Dan Zhang <danzh@google.com>

jmarantz · 2019-05-24T21:59:16Z

+
+OwnedImpl::OwnedImpl(OwnedImpl&& other)
+    : old_impl_(other.old_impl_), slices_(std::move(other.slices_)), length_(other.length_),
+      buffer_(std::move(other.buffer_)) {}


add unit tests for these new ctors/assign-ops in buffer_impl_test.cc. we should definitely not be just testing them from quic only.

jmarantz · 2019-05-24T22:00:25Z

+# wrapping Envoy data type to QUIC data type.
+#
+# See a detailed description of QUIC platform API dependency model at:
+# https://quiche.googlesource.com/quiche/+/refs/heads/master/quic/platform/api/README.md


btw this is excellent doc, thank you. Please render the dot diagram as a png also.

Signed-off-by: Dan Zhang <danzh@google.com>

jmarantz

Thanks for your patience while we worked through it! It's much simpler now.

jmarantz · 2019-05-29T15:03:42Z

+# wrapping Envoy data type to QUIC data type.
+#
+# See a detailed description of QUIC platform API dependency model at:
+# https://quiche.googlesource.com/quiche/+/refs/heads/master/quic/platform/api/README.md


What's the plan for fixing the diagram? Doesn't block this PR I suppose, but I am afraid if you don't do it someone will come along later and just delete the work you've done since it does not render as you intended.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-05-29T19:27:03Z

/retest

repokitteh-read-only · 2019-05-29T19:27:08Z

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #6400 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2019-05-29T20:42:02Z

merged with master branch. PTAL!

jmarantz

I think Matt's gotta look at this next.

mattklein123

LGTM from a skim. A couple of small questions. Thank you!

/wait

mattklein123 · 2019-05-30T18:13:02Z

+
+OwnedImpl::OwnedImpl(OwnedImpl&& other) : OwnedImpl() { move(other); }
+
+OwnedImpl& OwnedImpl::operator=(const OwnedImpl& other) {


Having the copy constructor and copy assignment operators can potentially lead to unintentional copies through C++ magic, so I would prefer to not have them. Is it possible to hide these behind explicit methods so they are very intentional?

I brought up the previous discussion about why we added copy contractor. Please refer to that thread.

I think the previous state was a strange explicit upcast which had more or less then same effect.

How about an explicit copyFrom() method instead, which you could call from the constructor body. Would that work?

Yes, +1 to an explicit method. I don't think we need the constructor/assignment overrides but could have explicit methods (possible static) which do what we need. you could also make these operators private, and then access them via public static factory methods.

/wait

Buffer::Instance already has similar method: add(). I switch to that one.

mattklein123 · 2019-05-30T18:14:37Z

+  Envoy::Buffer::RawSlice iovec;
+  uint64_t num_iov = single_slice_buffer_.reserve(length, &iovec, 1);
+  ASSERT(num_iov == 1);
+  // evbuffer may return a slice longer than needed, trim it to requested length.


nit: we are moving away from evbuffer internally. I would rephrase the comment to say this is how Envoy's buffer implementation works, etc.

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123

Nice, thanks!

danzh1989 added 2 commits March 14, 2019 17:11

QuicMemSlice v1

2792d28

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into memslice

ce398c8

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 requested review from alyssawilk and mattklein123 as code owners March 27, 2019 17:47

danzh1989 added 2 commits March 27, 2019 14:35

mem_slice_span

b906b0f

Signed-off-by: Dan Zhang <danzh@google.com>

fix conflict

ff8d74d

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 force-pushed the memslice2 branch from d2b0b32 to ff8d74d Compare March 27, 2019 18:41

danzh2010 requested review from htuch, snowp and zuercher as code owners March 27, 2019 18:41

danzh1989 added 3 commits March 27, 2019 14:48

Merge branch 'master' into memslice2

4f0d0e5

Signed-off-by: Dan Zhang <danzh@google.com>

add envoy_select_quiche

d2ff8b0

Signed-off-by: Dan Zhang <danzh@google.com>

fix ssl dependency

45705ff

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only Bot assigned wu-bin Mar 27, 2019

danzh1989 added 2 commits March 27, 2019 16:11

fix include of std library

95dcfab

Signed-off-by: Dan Zhang <danzh@google.com>

format

4cdb7a7

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 changed the title ~~quic-envoy: implement QuicMemSlice~~ quiche: implement QuicMemSlice Mar 28, 2019

wu-bin reviewed Mar 28, 2019

View reviewed changes

htuch requested review from jmarantz and removed request for alyssawilk, mattklein123, snowp and zuercher March 29, 2019 14:58

htuch assigned jmarantz Mar 29, 2019

remove dependancy on send buffer

1860c60

Signed-off-by: Dan Zhang <danzh@google.com>

jmarantz reviewed Mar 29, 2019

View reviewed changes

fix BUILD file format

1b27218

Signed-off-by: Dan Zhang <danzh@google.com>

wu-bin reviewed Mar 29, 2019

View reviewed changes

danzh2010 force-pushed the memslice2 branch from 6ed404d to 929e30b Compare May 24, 2019 21:50

Merge branch 'master' into memslice2

f4082d4

Signed-off-by: Dan Zhang <danzh@google.com>

jmarantz reviewed May 24, 2019

View reviewed changes

danzh1989 added 4 commits May 28, 2019 13:08

add test for OwnedImpl and comment

9c8865b

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into memslice2

1b70b1c

Signed-off-by: Dan Zhang <danzh@google.com>

fix comment

ccdc79d

Signed-off-by: Dan Zhang <danzh@google.com>

fix use after move

96503a4

Signed-off-by: Dan Zhang <danzh@google.com>

jmarantz reviewed May 28, 2019

View reviewed changes

Comment thread bazel/external/quiche.BUILD

Comment thread test/common/buffer/owned_impl_test.cc Outdated

comment quiche_copt

fd7926c

Signed-off-by: Dan Zhang <danzh@google.com>

jmarantz previously approved these changes May 29, 2019

View reviewed changes

jmarantz assigned mattklein123 May 29, 2019

Merge branch 'master' into memslice2

2e7645f

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed jmarantz’s stale review via 2e7645f May 29, 2019 18:38

jmarantz previously approved these changes May 29, 2019

View reviewed changes

mattklein123 requested changes May 30, 2019

View reviewed changes

repokitteh-read-only Bot added the waiting label May 30, 2019

refine comment

b11850c

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed jmarantz’s stale review via b11850c May 30, 2019 18:33

repokitteh-read-only Bot added waiting and removed waiting labels May 30, 2019

revert buffer_impl change

ca7fa75

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only Bot removed the waiting label May 30, 2019

format

085e247

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123 approved these changes May 30, 2019

View reviewed changes

mattklein123 merged commit 0c48eff into envoyproxy:master May 30, 2019


		inline void QuicPrintCommandLineFlagHelpImpl(const char* /usage/) {}

		// porting layer for QUICHE.


		OwnedImpl::OwnedImpl(OwnedImpl&& other) : OwnedImpl() { move(other); }

		OwnedImpl& OwnedImpl::operator=(const OwnedImpl& other) {

Conversation

danzh2010 commented Mar 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danzh2010 commented Mar 27, 2019

Uh oh!

wu-bin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch commented Mar 29, 2019

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

danzh2010 commented Mar 27, 2019 •

edited

Loading