Add alignment parameter to PartitionAllocHooks::AllocationOverrideHook. [chromium/src : main]
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Auto-Submit | +1 |
1 comment
I'm not perfectly happy with having to pull this sort of logic into alloc::Flags and passing into AllocInternal (feels a bit messy).
The other thing I thought about was patchset 4 where I passed a kNoRequiredAlignment, and just always did an std::max... but I also didn't like that as a solution since passing zero is weird...
So I went with this solution which
1) Avoids any impact on the NONE aligned path through the use of constexpr
2) Since it is only used internally on inlined functions doesn't really impact binary size.
Thoughts? Other potential approaches here?
Related details
- Kalvin Lee
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kalvin Lee (Gerrit)
Kalvin Lee voted and added 4 comments![]( "Open in Gerrit")
Votes added by Kalvin Lee
| Code-Review | +1 |
4 comments
Non-owners vote, with some questions (but nothing blocking). Adding GWP-ASan owners optimistically.
static_assert(!ContainsFlags(flags, AllocFlags::kAlignedAlloc));Could you document why this is being introduced?
kAlignedFreeForMemoryTool = 1 << 7, // Internal.Do we not need to adjust this at all? (If not, I would appreciate at least a comment here in this code review as to why not.)
std::optional<size_t> alignment);Random thought: the `std::optional` is good form, but I don't think anybody could ever request alignment of `0` --- maybe that could serve as a sentinel value. After all, that's what `GuardedPageAllocator::Allocate()` ends up using anyway.
Only applicable if the overhead is measurable, of course...
Related details
- Matthew Denton
- Stephen Nusko
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. |
Stephen Nusko (Gerrit)
Stephen Nusko added 3 comments![]( "Open in Gerrit")
static_assert(!ContainsFlags(flags, AllocFlags::kAlignedAlloc));Could you document why this is being introduced?
Yeah I hadn't really added comments. Wanted to get your thoughts on patchset 4 versus patchset 6 (see [comment](https://chromium-review.git.corp.google.com/c/chromium/src/+/7838366/comments/45f6865c_2604bfdb)).
I just wanted to block any other function from setting it EXCEPT AlignedAlloc (to prevent misuse of kAlignedAlloc. So I went to every callsite of AllocInternal and added a static assert, but I guess we could allow it for AllocInternalForTesting (not that anyone currently uses it), so you could write a test about it.
Note that I don't overly love this approach because in theory you could still add a new route to AllocInternal and not add the static assert... A bit annoying in terms of encapsulation and maintainablity, but I don't think we add new calls into AllocInternal much so perhaps it is okay.
Should I keep this or remove? Or enforce it in a different way?
(replying now but not resolving, will resolve once I add comments later today).
kAlignedFreeForMemoryTool = 1 << 7, // Internal.Do we not need to adjust this at all? (If not, I would appreciate at least a comment here in this code review as to why not.)
No, we could do so to make it symmetrical to avoid stepping on it later. It isn't required because GWP-ASAN does aligned allocation by just moving the pointer it returns INSIDE the 4KB page that it allocated. This means it doesn't need to do anything special to free. It just looks up the page and proceeds as normal.
This contrasts with using system APIs like aligned_alloc/aligned_free on windows, but we could make this consistent in case we need it for some future problem?
I can see pros and cons to both sides, what are your thoughts?
std::optional<size_t> alignment);Random thought: the `std::optional` is good form, but I don't think anybody could ever request alignment of `0` --- maybe that could serve as a sentinel value. After all, that's what `GuardedPageAllocator::Allocate()` ends up using anyway.
Only applicable if the overhead is measurable, of course...
What you described I did in patchset 4.
I think this should be fine. for two reasons
1) on non-aligned alloc this is always std::nullopt, and we only check the optional AFTER we've decided to sample for GWP-asan. Likely not going to impact perf.
2) On aligned alloc we do construct the optional, but again constexpr, so the compiler should determine that it just gets constructed in place and only gets checked after the fact.
Performance impact should be tiny. I can run a pinpoint on the two patchsets though. Though I'm not sure how much GWP ASAN impacts speedometer.
Take a look and let me know if you have strong preferences?
Related details
- Kalvin Lee
- Matthew Denton
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job mac-m1_mini_2020-perf/speedometer3 failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/105a5c76c90000
Related details
- Kalvin Lee
- Matthew Denton
- Stephen Nusko
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job mac-m1_mini_2020-perf/speedometer3 failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/16185171c90000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job mac-m1_mini_2020-perf/speedometer3 failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/16617c81c90000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/10795eeec90000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1470d6f9c90000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/14adf3f6c90000
Kalvin Lee (Gerrit)
Kalvin Lee added 4 comments![]( "Open in Gerrit")
Sustained non-owners vote. Matt, it's ready for you whenever you feel up for it 😊
static_assert(!ContainsFlags(flags, AllocFlags::kAlignedAlloc));Stephen NuskoCould you document why this is being introduced?
Yeah I hadn't really added comments. Wanted to get your thoughts on patchset 4 versus patchset 6 (see [comment](https://chromium-review.git.corp.google.com/c/chromium/src/+/7838366/comments/45f6865c_2604bfdb)).
I just wanted to block any other function from setting it EXCEPT AlignedAlloc (to prevent misuse of kAlignedAlloc. So I went to every callsite of AllocInternal and added a static assert, but I guess we could allow it for AllocInternalForTesting (not that anyone currently uses it), so you could write a test about it.
Note that I don't overly love this approach because in theory you could still add a new route to AllocInternal and not add the static assert... A bit annoying in terms of encapsulation and maintainablity, but I don't think we add new calls into AllocInternal much so perhaps it is okay.
Should I keep this or remove? Or enforce it in a different way?
(replying now but not resolving, will resolve once I add comments later today).
I personally like the idea of
``` c++
#include <concepts>
// ...
// narrative exposition to explain to kalvin why we are disallowing this
// blah blah blah
template <AllocFlags alloc_flags>
concept AlignedAllocDisallowed =
!ContainsFlags(alloc_flags, AllocFlags::kAlignedAlloc);
// ...
template <AllocFlags flags>
requires AlignedAllocDisallowed<flags>
PA_NOINLINE PA_MALLOC_FN void* PartitionRoot::AllocInternalForTesting(
// ...
```
Although it looks like this is generally [discouraged by the Google style guide](https://google.github.io/styleguide/cppguide.html#Concepts) (and not overridden by the Chromium style guide AFAICT), I personally think this is kind of okay since
- this _is_ an internal header, and
- encoding the requirement in the type of `AllocInternal*()` directly is kind of nice.
That said, I don't feel strongly about this, and the `static_assert`s do a fine job as they are.
I feel that `kNoRequiredAlignment` is a great name (very clear), but the thing we want it to do is kind of solved by `AllocFlags`. I have a slight preference for patchset 6 over patchset 4.
kAlignedFreeForMemoryTool = 1 << 7, // Internal.Stephen NuskoDo we not need to adjust this at all? (If not, I would appreciate at least a comment here in this code review as to why not.)
No, we could do so to make it symmetrical to avoid stepping on it later. It isn't required because GWP-ASAN does aligned allocation by just moving the pointer it returns INSIDE the 4KB page that it allocated. This means it doesn't need to do anything special to free. It just looks up the page and proceeds as normal.
This contrasts with using system APIs like aligned_alloc/aligned_free on windows, but we could make this consistent in case we need it for some future problem?
I can see pros and cons to both sides, what are your thoughts?
D'oh, I hadn't realized there wasn't a platform-agnostic aligned `free()` function 😞
This reads like an existing inconsistency, not being particularly exacerbated by this CL, so I'm inclined to let this go as is.
std::optional<size_t> alignment);Stephen NuskoRandom thought: the `std::optional` is good form, but I don't think anybody could ever request alignment of `0` --- maybe that could serve as a sentinel value. After all, that's what `GuardedPageAllocator::Allocate()` ends up using anyway.
Only applicable if the overhead is measurable, of course...
What you described I did in patchset 4.
I think this should be fine. for two reasons
1) on non-aligned alloc this is always std::nullopt, and we only check the optional AFTER we've decided to sample for GWP-asan. Likely not going to impact perf.
2) On aligned alloc we do construct the optional, but again constexpr, so the compiler should determine that it just gets constructed in place and only gets checked after the fact.Performance impact should be tiny. I can run a pinpoint on the two patchsets though. Though I'm not sure how much GWP ASAN impacts speedometer.
Take a look and let me know if you have strong preferences?
No strong preferences, and Speedometer3 lying flat is the strongest endorsement there is 😊
Related details
- Matthew Denton
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Denton (Gerrit)
Matthew Denton voted![]( "Open in Gerrit")
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Add alignment parameter to PartitionAllocHooks::AllocationOverrideHook.
A crash has been seen in Audio code which we suspect is due to it
requesting 32 alignment, but when it gets selected to be sampled by
GWP-ASAN this requirement is lost and it falls back to the default
GWP-ASAN alignment of 16.
The AllocationOverrideHook now receives the requested alignment. The
GWP-Asan sampling shim is updated to use this alignment when calling its
internal allocator, and a new test case is added to verify correct
handling of aligned allocations.
- M base/allocator/partition_allocator/src/partition_alloc/internal/partition_root_internal.h
- M base/allocator/partition_allocator/src/partition_alloc/partition_alloc_constants.h
- M base/allocator/partition_allocator/src/partition_alloc/partition_alloc_hooks.cc
- M base/allocator/partition_allocator/src/partition_alloc/partition_alloc_hooks.h
- M base/allocator/partition_allocator/src/partition_alloc/partition_alloc_unittest.cc
- M base/allocator/partition_allocator/src/partition_alloc/partition_root.cc
- M base/allocator/partition_allocator/src/partition_alloc/partition_root.h
- M components/gwp_asan/client/sampling_partitionalloc_shims.cc
- M components/gwp_asan/client/sampling_partitionalloc_shims_unittest.cc
Code-Review: +1 by Kalvin Lee, +1 by Matthew Denton
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |