[Memory Coordinator] Allow optional MemoryConsumerTraits with defaults [chromium/src : main]
Patrick Monette (Gerrit)
Patrick Monette added 1 comment![]( "Open in Gerrit")
Related details
- Andres Ricardo Perez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andres Ricardo Perez (Gerrit)
Andres Ricardo Perez voted and added 7 comments![]( "Open in Gerrit")
Votes added by Andres Ricardo Perez
| Code-Review | +1 |
7 comments
This look really cool! And I like the new ergonomics of the API for creating traits.
LGTM, with some questions on the testing.
// Prevent duplicate traits
static_assert(((internal::count_type_v<Args, Args...> == 1) && ...),
"Duplicate memory trait types provided.");Can we test this logic? Asking Gemini brings up no-compile tests as a tool, and I did find this: https://www.chromium.org/developers/testing/no-compile-tests/
And I see `/nc` files in [code search](https://source.chromium.org/search?q=f:%5C.nc$&ss=chromium%2Fchromium%2Fsrc:base%2F).
constexpr MemoryConsumerTraits(Args... args)
: supports_memory_limit(get<SupportsMemoryLimit>(args...)),Please fix this WARNING reported by ClangTidy: check: google-explicit-constructor
constructors that are callable with a single...
check: google-explicit-constructor
constructors that are callable with a single argument must be marked explicit to avoid unintentional implicit conversions (https://clang.llvm.org/extra/clang-tidy/checks/google/explicit-constructor.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-explicit-constructor` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
DECLARE_OPTIONAL_TRAIT(SupportsMemoryLimit, kYes);Nit: I would suggest adding a comment argument literal so that it's obvious to anyone creating a new client that these are the default values. It took me a bit to realize since the `#define` is in another header.
```suggestion
DECLARE_OPTIONAL_TRAIT(SupportsMemoryLimit, /*default_value=*/kYes);
```
// If T is not in Args..., this fires immediately.
static_assert((std::is_same_v<T, Args> || ...),
"A required memory trait is missing!");Same question as in `traits.cc`: Could we test this with a no-compile test?
// This test assumes each trait have 3 possible values only.Nit: has
const int kMaxValue = 3;If eventually there is a trait with more than 3 values, the test will still pass.
Is it possible to have an assert on the last iteration checking that it sets all traits to their corresponding `T::kMaxValue`?
If you'd rather not, we could have a comment in the traits definitions indicating that this test should be updated in that case.
Related details
- Patrick Monette
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Monette (Gerrit)
Patrick Monette voted and added 7 comments![]( "Open in Gerrit")
Votes added by Patrick Monette
| Commit-Queue | +1 |
7 comments
// Prevent duplicate traits
static_assert(((internal::count_type_v<Args, Args...> == 1) && ...),
"Duplicate memory trait types provided.");Can we test this logic? Asking Gemini brings up no-compile tests as a tool, and I did find this: https://www.chromium.org/developers/testing/no-compile-tests/
And I see `/nc` files in [code search](https://source.chromium.org/search?q=f:%5C.nc$&ss=chromium%2Fchromium%2Fsrc:base%2F).
Great idea. done.
constexpr MemoryConsumerTraits(Args... args)
: supports_memory_limit(get<SupportsMemoryLimit>(args...)),Please fix this WARNING reported by ClangTidy: check: google-explicit-constructor
constructors that are callable with a single...
check: google-explicit-constructor
constructors that are callable with a single argument must be marked explicit to avoid unintentional implicit conversions (https://clang.llvm.org/extra/clang-tidy/checks/google/explicit-constructor.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-explicit-constructor` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
N/A
DECLARE_OPTIONAL_TRAIT(SupportsMemoryLimit, kYes);Nit: I would suggest adding a comment argument literal so that it's obvious to anyone creating a new client that these are the default values. It took me a bit to realize since the `#define` is in another header.
```suggestion
DECLARE_OPTIONAL_TRAIT(SupportsMemoryLimit, /*default_value=*/kYes);
```
I simplified the implementation. I figured out that for required traits, I just had to use a normal constructor parameter 😐
Now default values are inline the enumeration, and I no longer need the macro.
// If T is not in Args..., this fires immediately.
static_assert((std::is_same_v<T, Args> || ...),
"A required memory trait is missing!");Same question as in `traits.cc`: Could we test this with a no-compile test?
Done
// This test assumes each trait have 3 possible values only.Patrick MonetteNit: has
Done
If eventually there is a trait with more than 3 values, the test will still pass.
Is it possible to have an assert on the last iteration checking that it sets all traits to their corresponding `T::kMaxValue`?
If you'd rather not, we could have a comment in the traits definitions indicating that this test should be updated in that case.
I reworked it so that 3 is no longer hardcoded, but generated from a type list.
Related details
- Andres Ricardo Perez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andres Ricardo Perez (Gerrit)
Andres Ricardo Perez voted Code-Review+1![]( "Open in Gerrit")
- Patrick Monette
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Monette (Gerrit)
Patrick Monette added 1 comment![]( "Open in Gerrit")
+chromium IPC reviews for mojo-related files.
Related details
- Chromium IPC Reviews
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: d...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): d...@chromium.org
Reviewer source(s):
d...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino voted Code-Review+1![]( "Open in Gerrit")
- Patrick Monette
Patrick Monette (Gerrit)
Patrick Monette added 1 comment![]( "Open in Gerrit")
+Gab for base stamp, and also parameter_pack.h hasn't been reviewed by an owner yet. And could I get owners override as well please.
Related details
- Gabriel Charette
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Charette (Gerrit)
Gabriel Charette added 1 comment![]( "Open in Gerrit")
constexpr MemoryConsumerTraits(EstimatedMemoryUsage estimated_memory_usage,Let's use the same mechanism as base::TaskTraits? (I like yours though but we should harmonize on the same one nonetheless?)
+ @etie...@chromium.org as C++ template traits expert
Related details
- Etienne Pierre-Doray
- Patrick Monette
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Monette (Gerrit)
Patrick Monette added 1 comment![]( "Open in Gerrit")
constexpr MemoryConsumerTraits(EstimatedMemoryUsage estimated_memory_usage,Let's use the same mechanism as base::TaskTraits? (I like yours though but we should harmonize on the same one nonetheless?)
+ @etie...@chromium.org as C++ template traits expert
Here is why I like my version for my use case:
The default value is written inline inside the enum. As someone that needs to look at the traits.h header to understand which value to pick for them, I think that's much easier to read.
The use of ParameterPack allows me to iterate over valid values for the generation of test cases in content/common/memory_coordinator/mojom/memory_consumer_traits_unittest.cc.
I agree that harmonizing would be great though.
Related details
- Etienne Pierre-Doray
- Gabriel Charette
Patrick Monette (Gerrit)
Patrick Monette added 1 comment![]( "Open in Gerrit")
constexpr MemoryConsumerTraits(EstimatedMemoryUsage estimated_memory_usage,Patrick MonetteLet's use the same mechanism as base::TaskTraits? (I like yours though but we should harmonize on the same one nonetheless?)
+ @etie...@chromium.org as C++ template traits expert
Here is why I like my version for my use case:
The default value is written inline inside the enum. As someone that needs to look at the traits.h header to understand which value to pick for them, I think that's much easier to read.The use of ParameterPack allows me to iterate over valid values for the generation of test cases in content/common/memory_coordinator/mojom/memory_consumer_traits_unittest.cc.
I agree that harmonizing would be great though.
I think the latter is more the "killer feature" as I could just use
trait_helpers::GetEnum<MyEnum, MyEnum::kDefaultValue>
Patrick Monette (Gerrit)
Patrick Monette added 2 comments![]( "Open in Gerrit")
Gab can you take another look.
Could use a owners override too please
constexpr MemoryConsumerTraits(EstimatedMemoryUsage estimated_memory_usage,Patrick MonetteLet's use the same mechanism as base::TaskTraits? (I like yours though but we should harmonize on the same one nonetheless?)
+ @etie...@chromium.org as C++ template traits expert
Patrick MonetteHere is why I like my version for my use case:
The default value is written inline inside the enum. As someone that needs to look at the traits.h header to understand which value to pick for them, I think that's much easier to read.The use of ParameterPack allows me to iterate over valid values for the generation of test cases in content/common/memory_coordinator/mojom/memory_consumer_traits_unittest.cc.
I agree that harmonizing would be great though.
I think the latter is more the "killer feature" as I could just use
trait_helpers::GetEnum<MyEnum, MyEnum::kDefaultValue>
- Gabriel Charette
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Charette (Gerrit)
Gabriel Charette added 2 comments![]( "Open in Gerrit")
// that are not specified.Give example of the expected call site pattern (does everything go in a {} list now? or just the optional args?)
base::MemoryConsumerTraits::ReleaseGCReferences::kYes);Even this feels verbose, can we make even more traits have defaults?
Related details
- Patrick Monette
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Monette (Gerrit)
Patrick Monette added 3 comments![]( "Open in Gerrit")
Give example of the expected call site pattern (does everything go in a {} list now? or just the optional args?)
Done
base::MemoryConsumerTraits::ReleaseGCReferences::kYes);Even this feels verbose, can we make even more traits have defaults?
I'm trying to strike a balance between verbosity and correctness.
A good amount of the existing traits will rarely be used, as they have one specific use case in mind. Having a default makes sense for them. It'll rarely be wrongly used.
The required traits I kept are things that will often vary per implementation of MemoryConsumer. I feel it would be dangerous to have a default for them, and have some consumers forget to specify it correctly.
It's verbose for the tests that don't care about them, but providing a constant with a descriptive name still allows for readable code.
Related details
- Gabriel Charette
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Charette (Gerrit)
Gabriel Charette voted and added 2 comments![]( "Open in Gerrit")
Votes added by Gabriel Charette
| Code-Review | +1 |
+Gab for base stamp, and also parameter_pack.h hasn't been reviewed by an owner yet. And could I get owners override as well please.
Doesn't look like you need O-O as dom@ already stamped the non-owned files
base::MemoryConsumerTraits::ReleaseGCReferences::kYes);Patrick MonetteEven this feels verbose, can we make even more traits have defaults?
I'm trying to strike a balance between verbosity and correctness.
A good amount of the existing traits will rarely be used, as they have one specific use case in mind. Having a default makes sense for them. It'll rarely be wrongly used.
The required traits I kept are things that will often vary per implementation of MemoryConsumer. I feel it would be dangerous to have a default for them, and have some consumers forget to specify it correctly.
It's verbose for the tests that don't care about them, but providing a constant with a descriptive name still allows for readable code.
Ack, I'll let you see how you want to evolve your defaults, I still feel this might be a bit much for simple consumers.
Related details
- Patrick Monette
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Monette (Gerrit)
Patrick Monette added 2 comments![]( "Open in Gerrit")
Thanks!
base::MemoryConsumerTraits::ReleaseGCReferences::kYes);Patrick MonetteEven this feels verbose, can we make even more traits have defaults?
Gabriel CharetteI'm trying to strike a balance between verbosity and correctness.
A good amount of the existing traits will rarely be used, as they have one specific use case in mind. Having a default makes sense for them. It'll rarely be wrongly used.
The required traits I kept are things that will often vary per implementation of MemoryConsumer. I feel it would be dangerous to have a default for them, and have some consumers forget to specify it correctly.
It's verbose for the tests that don't care about them, but providing a constant with a descriptive name still allows for readable code.
Ack, I'll let you see how you want to evolve your defaults, I still feel this might be a bit much for simple consumers.
For the most simple consumers (Let's say LRUCache of heap-allocated memory), we could have already defined traits that live in a utils.h header.
The plan is to migrate all and then see if there are common patterns that could benefit from having a constant.
Related details
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
namespace trait_helpers {I feel like this should be in traits_bag.h if under trait_helpers namespace (of otherwise keep it here and use something like internal)
Related details
- Patrick Monette
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Monette (Gerrit)
Patrick Monette added 1 comment![]( "Open in Gerrit")
I feel like this should be in traits_bag.h if under trait_helpers namespace (of otherwise keep it here and use something like internal)
Will use internal namesapce, given that there is already a concept of getting an optional enum in traits_bag.h and it would be confusing.
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |