-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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 all 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 |
|---|---|---|
| @@ -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 | ||
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.
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)
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.
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.
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.
SGTM!