-
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 23 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 |
|---|---|---|
|
|
@@ -103,6 +103,7 @@ envoy_cc_library( | |
| "quiche/spdy/platform/api/spdy_export.h", | ||
| "quiche/spdy/platform/api/spdy_flags.h", | ||
| "quiche/spdy/platform/api/spdy_logging.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", | ||
|
|
@@ -111,7 +112,10 @@ envoy_cc_library( | |
| ], | ||
| repository = "@envoy", | ||
| visibility = ["//visibility:public"], | ||
| deps = ["@envoy//source/extensions/quic_listeners/quiche/platform:spdy_platform_impl_lib"], | ||
| deps = [ | ||
| ":quiche_common_lib", | ||
| "@envoy//source/extensions/quic_listeners/quiche/platform:spdy_platform_impl_lib", | ||
| ], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
|
|
@@ -138,6 +142,68 @@ envoy_cc_library( | |
| deps = ["@envoy//source/extensions/quic_listeners/quiche/platform:spdy_platform_unsafe_arena_impl_lib"], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "spdy_core_alt_svc_wire_format_lib", | ||
| srcs = ["quiche/spdy/core/spdy_alt_svc_wire_format.cc"], | ||
| hdrs = ["quiche/spdy/core/spdy_alt_svc_wire_format.h"], | ||
| copts = quiche_copt, | ||
| repository = "@envoy", | ||
| visibility = ["//visibility:public"], | ||
| deps = [":spdy_platform"], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "spdy_core_header_block_lib", | ||
| srcs = ["quiche/spdy/core/spdy_header_block.cc"], | ||
| hdrs = ["quiche/spdy/core/spdy_header_block.h"], | ||
| copts = quiche_copt, | ||
| repository = "@envoy", | ||
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| ":spdy_platform", | ||
| ":spdy_platform_unsafe_arena_lib", | ||
| ], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "spdy_core_headers_handler_interface", | ||
| hdrs = ["quiche/spdy/core/spdy_headers_handler_interface.h"], | ||
| copts = quiche_copt, | ||
| repository = "@envoy", | ||
| visibility = ["//visibility:public"], | ||
| deps = [":spdy_platform"], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "spdy_core_protocol_lib", | ||
| hdrs = [ | ||
| "quiche/spdy/core/spdy_bitmasks.h", | ||
| "quiche/spdy/core/spdy_protocol.h", | ||
| ], | ||
| copts = quiche_copt, | ||
| repository = "@envoy", | ||
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| ":spdy_core_alt_svc_wire_format_lib", | ||
| ":spdy_core_header_block_lib", | ||
| ":spdy_platform", | ||
| ], | ||
| ) | ||
|
|
||
| envoy_cc_test_library( | ||
| name = "spdy_core_test_utils_lib", | ||
| srcs = ["quiche/spdy/core/spdy_test_utils.cc"], | ||
| hdrs = ["quiche/spdy/core/spdy_test_utils.h"], | ||
| copts = quiche_copt, | ||
| repository = "@envoy", | ||
| deps = [ | ||
| ":spdy_core_header_block_lib", | ||
| ":spdy_core_headers_handler_interface", | ||
| ":spdy_core_protocol_lib", | ||
| ":spdy_platform", | ||
| ], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "quic_platform", | ||
| srcs = [ | ||
|
|
@@ -157,6 +223,7 @@ envoy_cc_library( | |
| repository = "@envoy", | ||
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| ":quic_core_time_lib", | ||
| ":quic_platform_base", | ||
| "@envoy//source/extensions/quic_listeners/quiche/platform:quic_platform_impl_lib", | ||
| ], | ||
|
|
@@ -276,6 +343,7 @@ envoy_cc_library( | |
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| ":quic_platform_export", | ||
| ":quiche_common_lib", | ||
| "@envoy//source/extensions/quic_listeners/quiche/platform:quic_platform_base_impl_lib", | ||
| ], | ||
| ) | ||
|
|
@@ -301,6 +369,7 @@ envoy_cc_library( | |
| hdrs = ["quiche/quic/core/quic_error_codes.h"], | ||
| copts = quiche_copt, | ||
| repository = "@envoy", | ||
| visibility = ["//visibility:public"], | ||
| deps = [":quic_platform_export"], | ||
| ) | ||
|
|
||
|
|
@@ -367,6 +436,33 @@ envoy_cc_test_library( | |
| deps = [":epoll_server_platform"], | ||
| ) | ||
|
|
||
| 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", | ||
| visibility = ["//visibility:public"], | ||
| 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"], | ||
|
|
@@ -375,6 +471,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 = [ | ||
|
|
@@ -395,9 +502,21 @@ envoy_cc_test( | |
| deps = [":spdy_platform"], | ||
| ) | ||
|
|
||
| envoy_cc_test( | ||
| name = "spdy_core_header_block_test", | ||
| srcs = ["quiche/spdy/core/spdy_header_block_test.cc"], | ||
| copts = quiche_copt, | ||
| repository = "@envoy", | ||
| deps = [ | ||
| ":spdy_core_header_block_lib", | ||
| ":spdy_core_test_utils_lib", | ||
| ], | ||
| ) | ||
|
|
||
| envoy_cc_test( | ||
| name = "quic_platform_api_test", | ||
| srcs = [ | ||
| "quiche/quic/platform/api/quic_containers_test.cc", | ||
| "quiche/quic/platform/api/quic_endian_test.cc", | ||
| "quiche/quic/platform/api/quic_reference_counted_test.cc", | ||
| "quiche/quic/platform/api/quic_string_utils_test.cc", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| // 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. | ||
|
|
||
| // Add a dummy .cc file to make sure .h file is compiled. | ||
| #include "extensions/quic_listeners/quiche/platform/quiche_logging_impl.h" |
| 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,21 @@ | ||
| #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" | ||
| #include "absl/hash/hash.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,25 +1,22 @@ | ||
| #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" | ||
| #include "absl/hash/hash.h" | ||
|
|
||
| // The following includes should be: | ||
| // | ||
| // #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. | ||
|
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. Should this part of the comment be removed as well?
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. Oh, yes, they should. Done. |
||
| // TODO(mpw): fix includes to use full paths. | ||
| #include "spdy_string_impl.h" | ||
| #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 "extensions/quic_listeners/quiche/platform/spdy_string_impl.h" | ||
| #include "extensions/quic_listeners/quiche/platform/spdy_string_piece_impl.h" | ||
| #include "quiche/common/simple_linked_hash_map.h" | ||
|
|
||
| namespace spdy { | ||
|
|
||
|
|
@@ -31,8 +28,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>; | ||
|
|
||
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