-
Notifications
You must be signed in to change notification settings - Fork 5.5k
quiche: implement SimpleLinkedHashMap platform APIs #6782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
2b8bb98
4cdc39f
9a925f0
8d15493
3dd8ed9
b5b2a94
05df3ee
171d832
9e1bb66
af85c9c
55665ca
d2f9641
5fce010
7f1e153
3f84116
ca59554
f2c37c7
6dedd7e
fb42607
60b306a
e50d1a7
67ce36b
a4608c3
5e2d064
0db82cb
52a9269
0cacc00
8de6228
b15ec53
60f982e
b6056c2
5bcde48
c7e16e6
09f9f7a
792282d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,6 +106,7 @@ envoy_cc_library( | |
| "quiche/spdy/platform/api/spdy_estimate_memory_usage.h", | ||
| "quiche/spdy/platform/api/spdy_export.h", | ||
| "quiche/spdy/platform/api/spdy_flags.h", | ||
| "quiche/spdy/platform/api/spdy_macros.h", | ||
| "quiche/spdy/platform/api/spdy_mem_slice.h", | ||
| "quiche/spdy/platform/api/spdy_ptr_util.h", | ||
| "quiche/spdy/platform/api/spdy_string.h", | ||
|
|
@@ -147,6 +148,29 @@ envoy_cc_library( | |
| deps = ["@envoy//source/extensions/quic_listeners/quiche/platform:spdy_platform_unsafe_arena_impl_lib"], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "quiche_spdy_core_lib", | ||
| srcs = [ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: fold "srcs" and "hdrs" into a single line.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| "quiche/spdy/core/spdy_header_block.cc", | ||
| "quiche/spdy/core/spdy_test_utils.cc", | ||
| ], | ||
| hdrs = [ | ||
| "quiche/spdy/core/spdy_alt_svc_wire_format.h", | ||
| "quiche/spdy/core/spdy_bitmasks.h", | ||
| "quiche/spdy/core/spdy_header_block.h", | ||
| "quiche/spdy/core/spdy_headers_handler_interface.h", | ||
| "quiche/spdy/core/spdy_protocol.h", | ||
| "quiche/spdy/core/spdy_test_utils.h", | ||
| ], | ||
| copts = quiche_copt, | ||
| repository = "@envoy", | ||
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| ":spdy_platform", | ||
| ":spdy_platform_unsafe_arena_lib", | ||
| ], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "quic_platform", | ||
| srcs = ["quiche/quic/platform/api/quic_mutex.cc"] + envoy_select_quiche( | ||
|
|
@@ -365,6 +389,32 @@ envoy_cc_test_library( | |
| deps = ["@envoy//test/extensions/quic_listeners/quiche/platform:quic_platform_epoll_impl_lib"], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "quiche_common_platform", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM! |
||
| hdrs = [ | ||
| "quiche/common/platform/api/quiche_logging.h", | ||
| "quiche/common/platform/api/quiche_ptr_util.h", | ||
| "quiche/common/platform/api/quiche_unordered_containers.h", | ||
| ], | ||
| repository = "@envoy", | ||
| deps = ["@envoy//source/extensions/quic_listeners/quiche/platform:quiche_common_platform_impl_lib"], | ||
| ) | ||
|
|
||
| envoy_cc_test_library( | ||
| name = "quiche_common_platform_test", | ||
| hdrs = ["quiche/common/platform/api/quiche_test.h"], | ||
| repository = "@envoy", | ||
| deps = ["@envoy//test/extensions/quic_listeners/quiche/platform:quiche_common_platform_test_impl_lib"], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "quiche_common_lib", | ||
| hdrs = ["quiche/common/simple_linked_hash_map.h"], | ||
| repository = "@envoy", | ||
| visibility = ["//visibility:public"], | ||
| deps = [":quiche_common_platform"], | ||
| ) | ||
|
|
||
| envoy_cc_test( | ||
| name = "epoll_server_test", | ||
| srcs = ["quiche/epoll_server/simple_epoll_server_test.cc"], | ||
|
|
@@ -373,6 +423,17 @@ envoy_cc_test( | |
| deps = [":epoll_server_lib"], | ||
| ) | ||
|
|
||
| envoy_cc_test( | ||
| name = "quiche_common_test", | ||
| srcs = ["quiche/common/simple_linked_hash_map_test.cc"], | ||
| copts = quiche_copt, | ||
| repository = "@envoy", | ||
| deps = [ | ||
| ":quiche_common_lib", | ||
| ":quiche_common_platform_test", | ||
| ], | ||
| ) | ||
|
|
||
| envoy_cc_test( | ||
| name = "http2_platform_api_test", | ||
| srcs = [ | ||
|
|
@@ -396,6 +457,17 @@ envoy_cc_test( | |
| deps = [":spdy_platform"], | ||
| ) | ||
|
|
||
| envoy_cc_test( | ||
| name = "quiche_spdy_core_test", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: For spdy test, please name it spdy_${something}_test.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| srcs = ["quiche/spdy/core/spdy_header_block_test.cc"], | ||
| copts = quiche_copt, | ||
| repository = "@envoy", | ||
| deps = [ | ||
| ":quiche_spdy_core_lib", | ||
| ":spdy_platform", | ||
| ], | ||
| ) | ||
|
|
||
| envoy_cc_test( | ||
| name = "quic_platform_api_test", | ||
| srcs = envoy_select_quiche( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,6 +173,18 @@ envoy_cc_library( | |
| ]), | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "quiche_common_platform_impl_lib", | ||
| hdrs = [ | ||
| "quiche_logging_impl.h", | ||
| "quiche_ptr_util_impl.h", | ||
| "quiche_unordered_containers_impl.h", | ||
| ], | ||
| external_deps = ["abseil_node_hash_map"], | ||
| visibility = ["//visibility:public"], | ||
| deps = [":quic_platform_base_impl_lib"], | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still not fixed. This is really weird. Looking at the errors a few lines above: The file path is Why would clang_tidy CI do that?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CI image checkout repository to |
||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "quic_platform_sleep_impl_lib", | ||
| hdrs = ["quic_sleep_impl.h"], | ||
|
|
@@ -215,6 +227,7 @@ envoy_cc_library( | |
| ":quic_platform_logging_impl_lib", | ||
| ":string_utils_lib", | ||
| "//source/common/common:assert_lib", | ||
| "@com_googlesource_quiche//:quiche_common_lib", | ||
| ]), | ||
| ) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| #include "absl/container/inlined_vector.h" | ||
| #include "absl/container/node_hash_map.h" | ||
| #include "absl/container/node_hash_set.h" | ||
| #include "quiche/common/simple_linked_hash_map.h" | ||
|
|
||
| // NOLINT(namespace-envoy) | ||
|
|
||
|
|
@@ -26,14 +27,11 @@ using QuicUnorderedMapImpl = absl::node_hash_map<Key, Value, Hash>; | |
|
|
||
| template <typename Key, typename Hash> using QuicUnorderedSetImpl = absl::node_hash_set<Key, Hash>; | ||
|
|
||
| // TODO: implement | ||
| template <typename Key, typename Value, typename Hash> class QuicLinkedHashMapImpl {}; | ||
|
|
||
| // TODO: implement | ||
| template <typename Key, typename Value, int Size> class QuicSmallMapImpl {}; | ||
| template <typename Key, typename Value, typename Hash> | ||
| using QuicLinkedHashMapImpl = quiche::SimpleLinkedHashMap<Key, Value, Hash>; | ||
|
|
||
| // TODO: implement | ||
| template <typename T> class QuicIntervalSetImpl; | ||
| template <typename Key, typename Value, int Size> | ||
| using QuicSmallMapImpl = std::unordered_map<Key, Value>; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about absl::flat_hash_map, which is typically faster?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
|
||
| template <typename T> using QuicQueueImpl = std::queue<T>; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| #pragma once | ||
|
|
||
| // NOLINT(namespace-envoy) | ||
|
|
||
| // This file is part of the QUICHE platform implementation, and is not to be | ||
| // consumed or referenced directly by other Envoy code. It serves purely as a | ||
| // porting layer for QUICHE. | ||
|
|
||
| #include "extensions/quic_listeners/quiche/platform/quic_logging_impl.h" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| #pragma once | ||
|
|
||
| // NOLINT(namespace-envoy) | ||
|
|
||
| // This file is part of the QUICHE platform implementation, and is not to be | ||
| // consumed or referenced directly by other Envoy code. It serves purely as a | ||
| // porting layer for QUICHE. | ||
|
|
||
| #include <memory> | ||
|
|
||
| namespace quiche { | ||
|
|
||
| template <typename T, typename... Args> std::unique_ptr<T> QuicheMakeUniqueImpl(Args&&... args) { | ||
| return std::make_unique<T>(std::forward<Args>(args)...); | ||
| } | ||
|
|
||
| } // namespace quiche |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| #pragma once | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file is already in test/extensions/..., which is correct since it's test only.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, yes, this is not supposed to be here. |
||
|
|
||
| // NOLINT(namespace-envoy) | ||
|
|
||
| // This file is part of the QUICHE platform implementation, and is not to be | ||
| // consumed or referenced directly by other Envoy code. It serves purely as a | ||
| // porting layer for QUICHE. | ||
|
|
||
| #include "gmock/gmock.h" | ||
| #include "gtest/gtest.h" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| #pragma once | ||
|
|
||
| // NOLINT(namespace-envoy) | ||
|
|
||
| // This file is part of the QUICHE platform implementation, and is not to be | ||
| // consumed or referenced directly by other Envoy code. It serves purely as a | ||
| // porting layer for QUICHE. | ||
|
|
||
| #include "absl/container/node_hash_map.h" | ||
|
|
||
| namespace quiche { | ||
|
|
||
| // The default hasher used by hash tables. | ||
| template <typename Key> using QuicheDefaultHasherImpl = absl::Hash<Key>; | ||
|
|
||
| // 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>; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a particular reason to use absl::node_hash_map instead of absl::flat_hash_map?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| } // namespace quiche | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,11 @@ | ||
| #pragma once | ||
|
|
||
| // NOLINT(namespace-envoy) | ||
|
|
||
| // This file is part of the QUICHE platform implementation, and is not to be | ||
| // consumed or referenced directly by other Envoy code. It serves purely as a | ||
| // porting layer for QUICHE. | ||
|
|
||
| #include "absl/container/flat_hash_map.h" | ||
| #include "absl/container/flat_hash_set.h" | ||
| #include "absl/container/inlined_vector.h" | ||
|
|
@@ -14,12 +20,7 @@ | |
| // TODO(mpw): fix includes to use full paths. | ||
| #include "spdy_string_impl.h" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This TODO seems easy to fix, can you do it while we are here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| #include "spdy_string_piece_impl.h" | ||
|
|
||
| // NOLINT(namespace-envoy) | ||
|
|
||
| // This file is part of the QUICHE platform implementation, and is not to be | ||
| // consumed or referenced directly by other Envoy code. It serves purely as a | ||
| // porting layer for QUICHE. | ||
| #include "quiche/common/simple_linked_hash_map.h" | ||
|
|
||
| namespace spdy { | ||
|
|
||
|
|
@@ -31,8 +32,8 @@ using SpdyHashMapImpl = absl::flat_hash_map<KeyType, ValueType, Hash>; | |
| template <typename ElementType, typename Hasher, typename Eq> | ||
| using SpdyHashSetImpl = absl::flat_hash_set<ElementType, Hasher, Eq>; | ||
|
|
||
| // TODO: implement | ||
| template <typename Key, typename Value, typename Hash> class SpdyLinkedHashMapImpl {}; | ||
| template <typename Key, typename Value, typename Hash> | ||
| using SpdyLinkedHashMapImpl = quiche::SimpleLinkedHashMap<Key, Value, Hash>; | ||
|
|
||
| template <typename T, size_t N, typename A = std::allocator<T>> | ||
| using SpdyInlinedVectorImpl = absl::InlinedVector<T, N, A>; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| #pragma once | ||
|
|
||
| // NOLINT(namespace-envoy) | ||
|
|
||
| // This file is part of the QUICHE platform implementation, and is not to be | ||
| // consumed or referenced directly by other Envoy code. It serves purely as a | ||
| // porting layer for QUICHE. | ||
|
|
||
| #include "gmock/gmock.h" | ||
| #include "gtest/gtest.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed offline, can you split this library into smaller ones, mirroring the internal libraries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done