quiche: Implement QuicStreamBufferAllocator by danzh2010 · Pull Request #6550 · envoyproxy/envoy

danzh2010 · 2019-04-10T23:22:54Z

Implement this API with default SimpleBufferAllocator which use bare new and delete. In light of flag-enabled tcmalloc support, the default implementation will use tcmalloc underneath.

Roll up quiche tar ball to ba6354aa1b39f3d9788ead909ad3e678ac863938

Risk Level: low, not in use
Testing: Added simple test in test/extensions/quic_listeners/quiche/platform:quic_platform_test
Part of #2557

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

danzh2010 · 2019-04-10T23:27:16Z

/assign @wu-bin @jmarantz

wu-bin

The PR description says backed by tcmalloc, but it actually just uses the standard malloc/free functions, which may or may not be tcmalloc underneath?

Overall LGTM except some nits.

wu-bin · 2019-04-11T12:45:41Z

bazel/repository_locations.bzl

-        # Static snapshot of https://quiche.googlesource.com/quiche/+archive/4fbea5de9afdf30611b27afd54c45a596944f9c2.tar.gz
-        sha256 = "2cf9f5ea62a03ca0d8773fe4f56949b72c28ac5b1bcf43d850a571f4e32add2a",
-        urls = ["https://storage.googleapis.com/quiche-envoy-integration/4fbea5de9afdf30611b27afd54c45a596944f9c2.tar.gz"],
+        # Static snapshot of https://quiche.googlesource.com/quiche/+archive/553a966443258b103c97326f94b53b03b933361d.tar.gz


Can you help me update it to a version at or after https://quiche.googlesource.com/quiche/+/a27fd44c5c53a67f3fa772b805363eff039411d5?

wu-bin · 2019-04-11T12:48:23Z

bazel/external/quiche.BUILD

 )

+cc_library(
+    name = "quic_buffer_allocator_interface_lib",


How about "quic_buffer_allocator_lib", which is the name used internally?

+1 -- most build targets in envoy end with "_lib" or "_interface", not both.

wu-bin · 2019-04-11T12:49:26Z

source/extensions/quic_listeners/quiche/platform/quic_stream_buffer_allocator_impl.h

+// consumed or referenced directly by other Envoy code. It serves purely as a
+// porting layer for QUICHE.
+
+#include "quiche/quic/core/quic_buffer_allocator.h"


#include <stdlib.h> for malloc&free.

or <cstdlib>, which is guaranteed idempotent.

jmarantz

looks great modulo nits.

jmarantz · 2019-04-11T14:05:31Z

source/extensions/quic_listeners/quiche/platform/quic_stream_buffer_allocator_impl.h

+public:
+  ~QuicStreamBufferAllocatorImpl() override {}
+
+  char* New(size_t size) override { return static_cast<char*>(malloc(size)); }


above these put // QuickBufferAllocator per envoy convention.

jmarantz · 2019-04-11T14:08:02Z

test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc

+  char* p = allocator.New(1024);
+  EXPECT_NE(nullptr, p);
+  memset(p, 'a', 1024);
+  allocator.Delete(p);


You could also in this test ensure that the total allocated bytes was the same before and after this delete, via Memory::Stats::totalCurrentlyAllocated() before and after. See test/common/stats/thread_local_store_test.cc , test HeapStatsThreadLocalStoreTest.MemoryWithoutTls for an example. Only do that test if TestUtil::hasDeterministicMallocStats() is true.

jmarantz · 2019-04-11T14:09:46Z

@wu-bin that's the way tcmalloc works: it overrides malloc and free as well as operator new and delete.

It's worth noting that compiling with tcmalloc enabled is controlled by bazel flags, and I think it might not be enabled on some platform (Mac?)

wu-bin · 2019-04-11T14:13:16Z

@wu-bin that's the way tcmalloc works: it overrides malloc and free as well as operator new and delete.

@jmarantz That's true, but other memory allocators also overrides malloc/free etc.

It's worth noting that compiling with tcmalloc enabled is controlled by bazel flags, and I think it might not be enabled on some platform (Mac?)

jmarantz · 2019-04-11T14:33:30Z

@wu-bin RE "other memory allocators" -- you're right of course :) The implementation here doesn't really depend on tcmalloc and doesn't claim it does, except in the PR description. It's just delegating to the system memory allocator, which seems fine to me. @danzh2010 maybe just change the description to reflect that?

danzh2010 · 2019-04-11T15:32:13Z

@wu-bin that's the way tcmalloc works: it overrides malloc and free as well as operator new and delete.
Quic has another allcator: QuicSimpleBufferAllocator which use bare ::new and ::delete. Could this two ends up with doing the same thing underneath?

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

danzh2010

After the discussion about tcmalloc, it seems that the simplest way is to use quic's SimpleBufferAllocator as the impl. If tcmalloc turns out not to be sufficient, we can consider other buffer pool later.

danzh2010 · 2019-04-11T14:54:56Z

bazel/external/quiche.BUILD

 )

+cc_library(
+    name = "quic_buffer_allocator_interface_lib",


danzh2010 · 2019-04-11T14:58:00Z

bazel/repository_locations.bzl

-        # Static snapshot of https://quiche.googlesource.com/quiche/+archive/4fbea5de9afdf30611b27afd54c45a596944f9c2.tar.gz
-        sha256 = "2cf9f5ea62a03ca0d8773fe4f56949b72c28ac5b1bcf43d850a571f4e32add2a",
-        urls = ["https://storage.googleapis.com/quiche-envoy-integration/4fbea5de9afdf30611b27afd54c45a596944f9c2.tar.gz"],
+        # Static snapshot of https://quiche.googlesource.com/quiche/+archive/553a966443258b103c97326f94b53b03b933361d.tar.gz


danzh2010 · 2019-04-11T15:00:12Z

source/extensions/quic_listeners/quiche/platform/quic_stream_buffer_allocator_impl.h

+// consumed or referenced directly by other Envoy code. It serves purely as a
+// porting layer for QUICHE.
+
+#include "quiche/quic/core/quic_buffer_allocator.h"


danzh2010 · 2019-04-11T15:00:50Z

source/extensions/quic_listeners/quiche/platform/quic_stream_buffer_allocator_impl.h

+public:
+  ~QuicStreamBufferAllocatorImpl() override {}
+
+  char* New(size_t size) override { return static_cast<char*>(malloc(size)); }


danzh2010 · 2019-04-11T15:16:16Z

test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc

+  char* p = allocator.New(1024);
+  EXPECT_NE(nullptr, p);
+  memset(p, 'a', 1024);
+  allocator.Delete(p);


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

jmarantz · 2019-04-11T18:38:14Z

test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc

+  bool deterministic_stats = Envoy::Stats::TestUtil::hasDeterministicMallocStats();
+  size_t start_mem;
+  if (deterministic_stats) {
+    start_mem = Envoy::Memory::Stats::totalCurrentlyAllocated();


Might as well just assign start_mem conditionally.

jmarantz · 2019-04-11T18:38:21Z

test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc

+  size_t start_mem;
+  if (deterministic_stats) {
+    start_mem = Envoy::Memory::Stats::totalCurrentlyAllocated();
+    std::cerr << "start mem " << start_mem << "\n";


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

danzh2010 · 2019-04-11T21:05:10Z

/assign @mattklein123

jmarantz · 2019-04-11T21:07:00Z

test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc

+TEST_F(QuicPlatformTest, TestEnvoyQuicBufferAllocator) {
+  bool deterministic_stats = Envoy::Stats::TestUtil::hasDeterministicMallocStats();
+  const size_t start_mem =
+      deterministic_stats ? Envoy::Memory::Stats::totalCurrentlyAllocated() : 0;


actually you don't need to conditionalize this; totalCurrentlyAllocated() will return 0 if deterministic_stats is false.

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

danzh2010 requested review from alyssawilk and mattklein123 as code owners April 10, 2019 23:22

Implement QuicStreamBufferAllocator

fe99129

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

danzh2010 force-pushed the allocator branch from fe9f428 to fe99129 Compare April 10, 2019 23:26

repokitteh-read-only bot assigned jmarantz and wu-bin Apr 10, 2019

wu-bin previously approved these changes Apr 11, 2019

View reviewed changes

jmarantz previously approved these changes Apr 11, 2019

View reviewed changes

danzh1989 added 2 commits April 11, 2019 12:07

switch to use SimpleBufferAllocator

e94e659

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

Merge branch 'master' into allocator

1591f79

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

danzh2010 dismissed stale reviews from jmarantz and wu-bin via 1591f79 April 11, 2019 16:10

danzh2010 commented Apr 11, 2019

View reviewed changes

fix left over merge conflict

827b033

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

jmarantz previously approved these changes Apr 11, 2019

View reviewed changes

remove cerr

1115d12

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

danzh2010 dismissed jmarantz’s stale review via 1115d12 April 11, 2019 18:52

repokitteh-read-only bot assigned mattklein123 Apr 11, 2019

jmarantz reviewed Apr 11, 2019

View reviewed changes

uncondonize start_mem

01a2e33

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

mattklein123 approved these changes Apr 11, 2019

View reviewed changes

jmarantz merged commit 000d278 into envoyproxy:master Apr 12, 2019

danzh2010 changed the title ~~Implement QuicStreamBufferAllocator~~ quiche: Implement QuicStreamBufferAllocator Apr 15, 2019

Conversation

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

Uh oh!

Uh oh!

danzh2010 commented Apr 10, 2019

Uh oh!

wu-bin left a comment

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!

jmarantz Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz left a comment

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!

jmarantz commented Apr 11, 2019

Uh oh!

wu-bin commented Apr 11, 2019

Uh oh!

jmarantz commented Apr 11, 2019

Uh oh!

danzh2010 commented Apr 11, 2019

Uh oh!

danzh2010 left a comment

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!

danzh2010 commented Apr 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

danzh2010 commented Apr 10, 2019 •

edited

Loading

jmarantz Apr 11, 2019 •

edited

Loading