quiche: implement SimpleLinkedHashMap platform APIs by danzh2010 · Pull Request #6782 · envoyproxy/envoy

danzh2010 · 2019-05-02T16:59:38Z

Add platform implementations for quiche::SimpleLinkedHashMap. And enable simple_linked_hash_map_test.cc.
Finish TODOs in quic|spdy_containsers_impl.h to implement Quic|SpdyLinkedHashMap with SimpleLinkedHashMap.
Add a few spdy build targets and test target: spdy_core_header_block_test which tests SpdyHeaderBlock which uses SimpleLinkedHashMap.
Update tar ball to 7bf7c3c358eb954e463bde14ea27444f4bd8ea05.

Risk Level: low, not used
Testing: enabled quiche tests: simple_linked_hash_map_test.cc and spdy_header_block_test.cc
Part of #2557

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

danzh2010 · 2019-05-02T17:01:36Z

/assign @wu-bin

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

wu-bin · 2019-05-02T18:59:12Z

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

-// TODO: implement
-template <typename T> class QuicIntervalSetImpl;
+template <typename Key, typename Value, int Size>
+using QuicSmallMapImpl = std::unordered_map<Key, Value>;


How about absl::flat_hash_map, which is typically faster?

wu-bin · 2019-05-03T20:45:20Z

bazel/external/quiche.BUILD

    deps = ["@envoy//source/extensions/quic_listeners/quiche/platform:spdy_platform_unsafe_arena_impl_lib"],
 )

+envoy_cc_library(


As we discussed offline, can you split this library into smaller ones, mirroring the internal libraries?

LG modulo a super small nit: Can we remove the "spdy" in the middle of the library name? i.e. change to "spdy_core_alt_svc_wire_format_lib". Same for "spdy_core_spdy_header_block_lib", "spdy_core_spdy_headers_handler_interface", "spdy_core_spdy_protocol_lib" and "spdy_core_spdy_test_utils_lib".

wu-bin · 2019-05-03T20:46:41Z

bazel/external/quiche.BUILD

 )

+envoy_cc_library(
+    name = "quiche_common_platform",


nit: I'd prefer "quiche_platform", for similarity with (quic|spdy|http2)_platform.

Your call. (If you do change it to quiche_platform, please also change quiche_common_platform_test to quiche_platform_test, and quiche_common_platform_impl_lib to quiche_platform_impl_lib)

My concern is that if there are other directory added under quiche in the future, it will be hard to name those without specifying "common" here.

wu-bin · 2019-05-03T20:50:38Z

bazel/external/quiche.BUILD

 )

+envoy_cc_test(
+    name = "quiche_spdy_core_test",


nit: For spdy test, please name it spdy_${something}_test.

wu-bin · 2019-05-03T21:24:49Z

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

+
+// Similar to std::unordered_map, but with better performance and memory usage.
+template <typename Key, typename Value, typename Hash>
+using QuicheUnorderedMapImpl = absl::node_hash_map<Key, Value, Hash>;


Is there a particular reason to use absl::node_hash_map instead of absl::flat_hash_map?

flat_hash_map doesn't support key stability. Since this impl will probably replace QuicUnorderedMap which is used in several places as replacement of std::unordered_map. It will take some extra effort to investigate if those places requires key stability or not to switch to absl::flat_hash_map. And as we use node_hash_map in upstream, it's better to use the same class here to make sure future upstream change can be smoothly imported.

SGTM. In the long term we should probably rename it to QuicheNodeHashMap and add a QuicheFlatHashMap, or directly use the absl ones if that's allowed.

wu-bin · 2019-05-03T21:27:04Z

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

@@ -14,12 +20,7 @@
 // TODO(mpw): fix includes to use full paths.
 #include "spdy_string_impl.h"


This TODO seems easy to fix, can you do it while we are here?

wu-bin · 2019-05-03T21:29:58Z

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

@@ -0,0 +1,10 @@
+#pragma once


This file is already in test/extensions/..., which is correct since it's test only.
Can you remove this file?

Ah, yes, this is not supposed to be here.

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

wu-bin

Looks great, just some nits on naming:)

wu-bin · 2019-05-06T23:42:10Z

bazel/external/quiche.BUILD

+    copts = quiche_copt,
+    repository = "@envoy",
+    visibility = ["//visibility:public"],
+    deps = [


nit: fold deps into a single line.

wu-bin · 2019-05-06T23:42:33Z

bazel/external/quiche.BUILD

+
+envoy_cc_library(
+    name = "spdy_core_spdy_header_block_lib",
+    srcs = [


nit: fold "srcs" and "hdrs" into a single line.

wu-bin · 2019-05-06T23:43:40Z

bazel/external/quiche.BUILD

+
+envoy_cc_test_library(
+    name = "spdy_core_spdy_test_utils_lib",
+    srcs = [


nit: fold "srcs" and "hdrs" into single lines.

wu-bin · 2019-05-06T23:43:58Z

bazel/external/quiche.BUILD

 )

+envoy_cc_library(
+    name = "quiche_common_platform",


wu-bin · 2019-05-06T23:45:47Z

bazel/external/quiche.BUILD

 )

+envoy_cc_test(
+    name = "spdy_header_block_test",


nit: Rename to spdy_core_header_block_test?

wu-bin · 2019-05-06T23:50:03Z

bazel/external/quiche.BUILD

    deps = ["@envoy//source/extensions/quic_listeners/quiche/platform:spdy_platform_unsafe_arena_impl_lib"],
 )

+envoy_cc_library(


LG modulo a super small nit: Can we remove the "spdy" in the middle of the library name? i.e. change to "spdy_core_alt_svc_wire_format_lib". Same for "spdy_core_spdy_header_block_lib", "spdy_core_spdy_headers_handler_interface", "spdy_core_spdy_protocol_lib" and "spdy_core_spdy_test_utils_lib".

wu-bin · 2019-05-06T23:57:37Z

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

+
+// Similar to std::unordered_map, but with better performance and memory usage.
+template <typename Key, typename Value, typename Hash>
+using QuicheUnorderedMapImpl = absl::node_hash_map<Key, Value, Hash>;


SGTM. In the long term we should probably rename it to QuicheNodeHashMap and add a QuicheFlatHashMap, or directly use the absl ones if that's allowed.

wu-bin · 2019-05-07T00:01:03Z

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

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


I think the two includes will be reordered if you run "fix_format".

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

wu-bin

Looks great to me. I took a look at the clang-tidy failure again, but still can't figure out why. @htuch @mpwarres : Any idea?

htuch · 2019-05-08T10:18:12Z

@wu-bin not sure; @lizan probably has a better idea. Random guesses include possibly missing Bazel deps or weird symlink interactions.

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

danzh2010 · 2019-05-09T21:54:02Z

@lizan any idea about the clang_tidy failure?
bazel-out/k8-fastbuild/genfiles/external/com_googlesource_quiche/quiche/common/platform/api/quiche_logging.h:4:10: error: 'extensions/quic_listeners/quiche/platform/quiche_logging_impl.h' file not found [clang-diagnostic-error]
#include "extensions/quic_listeners/quiche/platform/quiche_logging_impl.h"
^

Apparently not missing files in deps, otherwise other ci would have failed.

lizan · 2019-05-09T22:00:47Z

source/extensions/quic_listeners/quiche/platform/BUILD

+        "abseil_node_hash_map",
+    ],
+    visibility = ["//visibility:public"],
+    deps = [":quic_platform_base_impl_lib"],


add :quic_platform_logging_impl_lib to fix clang-tidy, bazel doesn't build header files only target explicitly but clang-tidy checks whether you added dependencies correctly.

Woops, just realized another problem that quic_logging_impl.h is duplicated in both quic_platform_logging_impl_lib and quic_platform_base_impl_lib.

Fixed. Thanks for looking into this problem!

Still not fixed. This is really weird. Looking at the errors a few lines above:
Error while processing /source/source/extensions/quic_listeners/quiche/platform/quic_containers_impl.h.
10294 warnings and 1 error generated.
Error while processing /source/test/extensions/quic_listeners/quiche/platform/quiche_test_impl.h.
16998 warnings and 1 error generated.
Error while processing /source/source/extensions/quic_listeners/quiche/platform/quiche_logging_impl.h.
23702 warnings and 1 error generated.
Error while processing /source/source/extensions/quic_listeners/quiche/platform/quiche_logging_impl.cc.
31722 warnings and 1 error generated.
Error while processing /source/source/extensions/quic_listeners/quiche/platform/spdy_containers_impl.h.
37223 warnings and 1 error generated.
Error while processing /source/source/extensions/quic_listeners/quiche/platform/spdy_string_utils_impl.h.
39075 warnings and 1 error generated.
Error while processing /source/source/extensions/quic_listeners/quiche/platform/quiche_ptr_util_impl.h.
43844 warnings and 1 error generated.
Error while processing /source/source/extensions/quic_listeners/quiche/platform/quiche_unordered_containers_impl.h.
warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil [abseil-no-internal-dependencies]
bazel-out/k8-fastbuild/genfiles/external/com_googlesource_quiche/quiche/common/platform/api/quiche_logging.h:4:10: error: 'extensions/quic_listeners/quiche/platform/quiche_logging_impl.h' file not found [clang-diagnostic-error]
#include "extensions/quic_listeners/quiche/platform/quiche_logging_impl.h"

The file path is /source/source/extensions/quic_listeners/quiche/platform/... or /source/test/extensions/quic_listeners/quiche/platform/.... There is a /source added to the actual file path.

Why would clang_tidy CI do that?

CI image checkout repository to /source so the path is correct.

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

danzh2010 · 2019-05-09T22:54:23Z

/assign @mattklein123

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

danzh2010 · 2019-05-21T18:34:54Z

previous clang_tidy failure was caused by a circular dependency. Fixed now. PTAL

alyssawilk

lgtm modulo one nit

alyssawilk · 2019-05-21T18:55:35Z

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

-// #include "extensions/quic_listeners/quiche/platform/spdy_string_impl.h"
-// #include "extensions/quic_listeners/quiche/platform/spdy_string_piece_impl.h"
 //
 // However, for some reason, bazel.clang_tidy cannot resolve the files when specified this way.


Should this part of the comment be removed as well?

Oh, yes, they should. Done.

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

danzh1989 added 7 commits April 16, 2019 19:10

Implement SpdyUnsafeArena using SpdySimpleArena.

2b8bb98

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

de-dup header file

4cdc39f

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

fix macros, blocked by linked_hash_map

9a925f0

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

update tar ball

8d15493

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

Merge branch 'master' into spdy_arena

3dd8ed9

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

plug in SimpleLinkedHashMap

b5b2a94

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

format

05df3ee

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

danzh2010 requested review from alyssawilk and mattklein123 as code owners May 2, 2019 16:59

Merge branch 'master' into spdy_arena

171d832

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

repokitteh-read-only bot assigned wu-bin May 2, 2019

danzh1989 added 5 commits May 2, 2019 14:01

add visibility

9e1bb66

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

fix depenedency

af85c9c

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

add visibility

55665ca

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

add quic_containers_test

d2f9641

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

inline some impl functions

5fce010

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

wu-bin reviewed May 3, 2019

View reviewed changes

danzh1989 added 3 commits May 6, 2019 15:18

break down spdy_core_lib

7f1e153

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

Merge branch 'master' into spdy_arena

3f84116

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

fix #include

ca59554

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

wu-bin reviewed May 6, 2019

View reviewed changes

wu-bin reviewed May 7, 2019

View reviewed changes

fix format

f2c37c7

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

wu-bin previously approved these changes May 7, 2019

View reviewed changes

Merge branch 'master' into spdy_arena

6dedd7e

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

danzh2010 dismissed wu-bin’s stale review via 6dedd7e May 8, 2019 20:17

danzh1989 added 2 commits May 8, 2019 17:17

fix test deps

fb42607

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

format

60b306a

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

fix deps agains

e50d1a7

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

lizan reviewed May 9, 2019

View reviewed changes

fix deps

67ce36b

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

repokitteh-read-only bot assigned mattklein123 May 9, 2019

mattklein123 added the waiting label May 13, 2019

add dummy .cc file

a4608c3

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

repokitteh-read-only bot removed the waiting label May 15, 2019

danzh1989 added 3 commits May 17, 2019 15:49

add dummy function to logging_impl.h

5e2d064

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

rename dummy

0db82cb

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

inline dummy

52a9269

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

mattklein123 assigned alyssawilk and unassigned mattklein123 May 19, 2019

danzh1989 added 7 commits May 20, 2019 12:15

take out quic_logging

0cacc00

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

Merge branch 'master' into spdy_arena

8de6228

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

remove quiche_logging_impl.cc

b15ec53

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

move quic_logging into base

60f982e

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

break circular deps

b6056c2

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

format

5bcde48

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

Merge branch 'master' into spdy_arena

c7e16e6

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

alyssawilk reviewed May 21, 2019

View reviewed changes

danzh1989 added 2 commits May 21, 2019 16:59

remove out-dated comment

09f9f7a

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

format

792282d

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

alyssawilk approved these changes May 23, 2019

View reviewed changes

alyssawilk merged commit d3e35b6 into envoyproxy:master May 23, 2019

		@@ -14,12 +20,7 @@
		// TODO(mpw): fix includes to use full paths.
		#include "spdy_string_impl.h"

Conversation

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

Uh oh!

Uh oh!

danzh2010 commented May 2, 2019

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!

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!

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!

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!

wu-bin left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented May 8, 2019

Uh oh!

danzh2010 commented May 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented May 2, 2019 •

edited

Loading