umahp: Record residency status as tags in heap profiles [chromium/src : main]
Joe Mason (Gerrit)
Joe Mason added 1 comment![]( "Open in Gerrit")
Joe Masonhey joe, what do you think of the architecture here?
Interesting approach. Architecture LGTM, I haven't had time to review the impl in depth though. Sorry for the delay.
Related details
- Sean Maher
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason added 9 comments![]( "Open in Gerrit")
std::vector<trace_event::ProcessMemoryDump::MemoryRange> ranges;Nit: `reserve(active_samples.size())` before the for loop?
size_t aligned_size =Nit: Took me a while to figure out this calculation. Please add comments explaining it. (It's the distance from aligned_addr to the end of the allocation, rounded up to multiples of page_size, right?)
resident_ranges;Nit: could move this declaration up with `ranges` and just check the feature once.
sample.is_resident = true;Since a 0-byte allocation can't be real, it seems like this should be false?
sample.is_resident = !resident_ranges.has_value() ||Likewise, if we can't find a resident range, shouldn't is_resident be false? Or just left as nullopt?
raw_ptr<void> start_address;Because this isn't actually holding allocated memory, raw_ptr is extra overhead. I'd use uintptr_t for this, or RAW_PTR_EXCLUSION if there would be too much casting. (See https://chromium.googlesource.com/chromium/src/+/main/base/memory/raw_ptr.md#pointers-in-locations-other-than-fields, "Pointers whose addresses are used only as identifiers".)
ProcessMemoryDump::CountResidentBytes(base::span<MemoryRange> ranges) {I think the semantics of this function would make more sense if MemoryRange had `optional<size_t> resident_bytes`, with nullopt for unknown. Then the function would just return void, and each range in `ranges` could succeed or fail separately:
```
for (auto& range : ranges) {
...
bool failure = false;
while (offset < range.size) {
...
if (failure) {
PLOG(ERROR) << "CountResidentBytes"; // Or just remove the log.
break;
}
range.resident_bytes = total_resident_pages;
offset += kMaxChunkSize;
}
}
```
Then sampling_heap_profiler.cc would set `is_resident = ranges[range_idx++].value_or(0) > 0`, or something like that.
while (offset < range.size) {Seems like this is doing extra work for the heap profiler case - it could stop as soon as any resident bytes are found in the range, without having to iterate through the whole thing. Maybe give MemoryRange an `optional<bool>` instead of `optional<size_t>`, and add a flag param to say whether to only fill in the bools or also calculate `total_resident_pages`?
return lhs.is_resident < rhs.is_resident;Nit: the standard way to do this is `std::tie(lhs.stack, lhs.is_resident) < std::tie(rhs.stack, rhs.is_resident)`. I *think* your code is equivalent, but I'm too tired to be certain right now, and using std::tie would be easier for other people reading this later.
Related details
- Sean Maher
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. |
Sean Maher (Gerrit)
Sean Maher added 9 comments![]( "Open in Gerrit")
std::vector<trace_event::ProcessMemoryDump::MemoryRange> ranges;Nit: `reserve(active_samples.size())` before the for loop?
Done
Nit: Took me a while to figure out this calculation. Please add comments explaining it. (It's the distance from aligned_addr to the end of the allocation, rounded up to multiples of page_size, right?)
yep. hopefully made it more clear
Nit: could move this declaration up with `ranges` and just check the feature once.
Done
Since a 0-byte allocation can't be real, it seems like this should be false?
i actually disagree, but i left it as nullopt.
Likewise, if we can't find a resident range, shouldn't is_resident be false? Or just left as nullopt?
ok, left it as nullopt.
Because this isn't actually holding allocated memory, raw_ptr is extra overhead. I'd use uintptr_t for this, or RAW_PTR_EXCLUSION if there would be too much casting. (See https://chromium.googlesource.com/chromium/src/+/main/base/memory/raw_ptr.md#pointers-in-locations-other-than-fields, "Pointers whose addresses are used only as identifiers".)
Done
ProcessMemoryDump::CountResidentBytes(base::span<MemoryRange> ranges) {I think the semantics of this function would make more sense if MemoryRange had `optional<size_t> resident_bytes`, with nullopt for unknown. Then the function would just return void, and each range in `ranges` could succeed or fail separately:
```
for (auto& range : ranges) {
...
bool failure = false;
while (offset < range.size) {
...
if (failure) {
PLOG(ERROR) << "CountResidentBytes"; // Or just remove the log.
break;
}
range.resident_bytes = total_resident_pages;
offset += kMaxChunkSize;
}
}
```Then sampling_heap_profiler.cc would set `is_resident = ranges[range_idx++].value_or(0) > 0`, or something like that.
mostly done, but with std::nullopt propagation instead.
Seems like this is doing extra work for the heap profiler case - it could stop as soon as any resident bytes are found in the range, without having to iterate through the whole thing. Maybe give MemoryRange an `optional<bool>` instead of `optional<size_t>`, and add a flag param to say whether to only fill in the bools or also calculate `total_resident_pages`?
I am redoing this to use all the information.
Nit: the standard way to do this is `std::tie(lhs.stack, lhs.is_resident) < std::tie(rhs.stack, rhs.is_resident)`. I *think* your code is equivalent, but I'm too tired to be certain right now, and using std::tie would be easier for other people reading this later.
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason added 3 comments![]( "Open in Gerrit")
if (ranges[range_idx].resident_bytes.has_value() && sample.size > 0) {Nit: >0 check is redundant, since size is unsigned.
sample.resident_total = static_cast<size_t>(Nit: use `base::checked_cast` here? It looks like the llround should never overflow `size_t` since all the inputs are size_t, but that would verify it. (Or `base::saturated_cast` if it's possible for the result to be large, and we just want that to set resident_total to the max.)
(*ranges[range_idx].resident_bytes) / sample.size));Nit: putting `resident_bytes` in a temporary would make this more clear. I miscounted the parens when I first read this and thought there was an order-of-operations bug.
Related details
- Sean Maher
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. |
Joe Mason (Gerrit)
Joe Mason added 11 comments![]( "Open in Gerrit")
profiler->SetSamplingInterval(1024);Nit: should also create a `PoissonAllocationSampler::ScopedSuppressRandomnessForTesting` object. That will make it sample at EXACTLY 1024 bytes, to avoid test flakes if it happens to sample more rarely than expected.
if (sample.resident_total.has_value()) {Optional nit: When I first read this I thought the `if` meant the `resident_total` might not actually be set during the test, which confused me. It'd be more clear to use `ASSERT_TRUE(has_value())` which returns early if it's false, or gmock's EXPECT_THAT and Optional:
`EXPECT_THAT(sample.resident_total, ::testing::Optional(::testing::Gt(0u)))`
max_mapped_size = std::max(max_mapped_size, range.size);I don't think this is always the right max: for each entry in the range the `mapped_size` below is `aligned_end - aligned_start` and here it's just `end - start`. That could mean that an allocation that happens to be near the end of a page would span 2 pages, but this would only return 1 page. Which in turn could mean that `page_count` calculated below could be higher than the size of `vec`.
(Also since this doesn't correspond exactly to `mapped_size` it should have a different name.)
How about just dropping this calculation and always allocating a vector for kMaxChunkSize? It would be less efficient when querying a small amount of already-aligned memory, but that's probably not significant compared to the cost of actually calling mincore / QueryWorkingSetEx.
Or you could repeat the calculation to find the fully aligned `mapped_size` in this loop.
A trivial example of this: `ranges` has one entry, with a valid `start_pointer` and size `0`. `max_vec_size` will be 0, but `aligned_end` - `aligned_start` gives 1 page.
for (auto& range : ranges) {Nit: might want to check for `range.size` 0 and continue immediately. Otherwise it'll do the work to query 1 page even though the accumulate function will always find 0 overlap. As long as `vec` isn't empty, that'd work, it's just inefficient.
base::bits::AlignDown(start_pointer, page_size);Nice! I didn't know about that func.
uintptr_t chunk_start = aligned_start + offset;Nit: I think these calculations only work if kMaxChunkSize is an exact multiple of page_size. Otherwise `chunk_start` would end up within a page, and `page_addr` below wouldn't make sense. Can you add a CHECK for that at the start of the function, just in case?
[[maybe_unused]] auto accumulate_page_if_resident =Nit: is the `maybe_unused` still needed with the `std:ignore` in the fuschia branch?
UNSAFE_BUFFERS(CountResidentBytes(base::span(&range, 1u)));Nit: `base::span_from_ref(range)` won't need the UNSAFE_BUFFERS annotation.
const uintptr_t start_ptr =What are all these changes for? Nothing in the patch calls CountResidentBytesInSharedMemory so they can't be necessary for it, and the original code didn't call CountResidentBytes so shouldn't be affected by any changes to it.
std::ranges::fill(memory2, 0u);Nit: let's add a range with a size 0 alloc, to test the edge case.
Also maybe a range with deallocated memory that should have 0 resident bytes. (Unless unmapping doesn't always mark the memory non-resident immediately, in which case that would be flaky.)
Oh, another good test would be an unaligned pointer partway into one of the maps.
ASSERT_EQ(res1.value(), kDirtyMemorySize + page_size / 2);I assume this is only needed because of the changes to CountResidentBytesInSharedMemory?
Sean Maher (Gerrit)
Sean Maher added 13 comments![]( "Open in Gerrit")
if (ranges[range_idx].resident_bytes.has_value() && sample.size > 0) {Nit: >0 check is redundant, since size is unsigned.
Done
Nit: use `base::checked_cast` here? It looks like the llround should never overflow `size_t` since all the inputs are size_t, but that would verify it. (Or `base::saturated_cast` if it's possible for the result to be large, and we just want that to set resident_total to the max.)
Done
Nit: putting `resident_bytes` in a temporary would make this more clear. I miscounted the parens when I first read this and thought there was an order-of-operations bug.
Done
Nit: should also create a `PoissonAllocationSampler::ScopedSuppressRandomnessForTesting` object. That will make it sample at EXACTLY 1024 bytes, to avoid test flakes if it happens to sample more rarely than expected.
Done
Optional nit: When I first read this I thought the `if` meant the `resident_total` might not actually be set during the test, which confused me. It'd be more clear to use `ASSERT_TRUE(has_value())` which returns early if it's false, or gmock's EXPECT_THAT and Optional:
`EXPECT_THAT(sample.resident_total, ::testing::Optional(::testing::Gt(0u)))`
Done
I don't think this is always the right max: for each entry in the range the `mapped_size` below is `aligned_end - aligned_start` and here it's just `end - start`. That could mean that an allocation that happens to be near the end of a page would span 2 pages, but this would only return 1 page. Which in turn could mean that `page_count` calculated below could be higher than the size of `vec`.
(Also since this doesn't correspond exactly to `mapped_size` it should have a different name.)
How about just dropping this calculation and always allocating a vector for kMaxChunkSize? It would be less efficient when querying a small amount of already-aligned memory, but that's probably not significant compared to the cost of actually calling mincore / QueryWorkingSetEx.
Or you could repeat the calculation to find the fully aligned `mapped_size` in this loop.
A trivial example of this: `ranges` has one entry, with a valid `start_pointer` and size `0`. `max_vec_size` will be 0, but `aligned_end` - `aligned_start` gives 1 page.
right, i think this fixes it.
Nit: might want to check for `range.size` 0 and continue immediately. Otherwise it'll do the work to query 1 page even though the accumulate function will always find 0 overlap. As long as `vec` isn't empty, that'd work, it's just inefficient.
Done
Nit: I think these calculations only work if kMaxChunkSize is an exact multiple of page_size. Otherwise `chunk_start` would end up within a page, and `page_addr` below wouldn't make sense. Can you add a CHECK for that at the start of the function, just in case?
Marked as resolved.
Nit: is the `maybe_unused` still needed with the `std:ignore` in the fuschia branch?
Done
UNSAFE_BUFFERS(CountResidentBytes(base::span(&range, 1u)));Nit: `base::span_from_ref(range)` won't need the UNSAFE_BUFFERS annotation.
Done
const uintptr_t start_ptr =What are all these changes for? Nothing in the patch calls CountResidentBytesInSharedMemory so they can't be necessary for it, and the original code didn't call CountResidentBytes so shouldn't be affected by any changes to it.
getting rid of divergent behavior between two functions that are almost identically named. i could keep the divergent behavior, and just add a comment if you'd prefer this. I don't mind, just assumed it'd be better to have both deliver similar APIs
Nit: let's add a range with a size 0 alloc, to test the edge case.
Also maybe a range with deallocated memory that should have 0 resident bytes. (Unless unmapping doesn't always mark the memory non-resident immediately, in which case that would be flaky.)
Oh, another good test would be an unaligned pointer partway into one of the maps.
Done
ASSERT_EQ(res1.value(), kDirtyMemorySize + page_size / 2);I assume this is only needed because of the changes to CountResidentBytesInSharedMemory?
- Joe Mason
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. |
Joe Mason (Gerrit)
Joe Mason voted and added 2 comments![]( "Open in Gerrit")
Votes added by Joe Mason
| Code-Review | +1 |
2 comments
LGTM if the CountResidentBytesInSharedMemory are removed (I haven't reviewed them).
const uintptr_t start_ptr =Sean MaherWhat are all these changes for? Nothing in the patch calls CountResidentBytesInSharedMemory so they can't be necessary for it, and the original code didn't call CountResidentBytes so shouldn't be affected by any changes to it.
getting rid of divergent behavior between two functions that are almost identically named. i could keep the divergent behavior, and just add a comment if you'd prefer this. I don't mind, just assumed it'd be better to have both deliver similar APIs
Let's put a TODO comment "let CountResidentBytesInSharedMemory support unaligned addresses" in this patch, and move the changes to it to a followup. That way if there's a mistake in this fiddly math, it can be reverted separately.
Related details
- Sean Maher
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. |
Joe Mason (Gerrit)
Joe Mason added 2 comments![]( "Open in Gerrit")
LGTM if the CountResidentBytesInSharedMemory are removed (I haven't reviewed them).
And I see I'm not OWNER of that file anyway, so if Etienne reviews and approves those changes, go ahead and commit.
ASSERT_EQ(res1.value(), kDirtyMemorySize + page_size / 2);Sean MaherI assume this is only needed because of the changes to CountResidentBytesInSharedMemory?
yes
Acknowledged
Sean Maher (Gerrit)
Sean Maher added 1 comment![]( "Open in Gerrit")
const uintptr_t start_ptr =Sean MaherWhat are all these changes for? Nothing in the patch calls CountResidentBytesInSharedMemory so they can't be necessary for it, and the original code didn't call CountResidentBytes so shouldn't be affected by any changes to it.
Joe Masongetting rid of divergent behavior between two functions that are almost identically named. i could keep the divergent behavior, and just add a comment if you'd prefer this. I don't mind, just assumed it'd be better to have both deliver similar APIs
Let's put a TODO comment "let CountResidentBytesInSharedMemory support unaligned addresses" in this patch, and move the changes to it to a followup. That way if there's a mistake in this fiddly math, it can be reverted separately.
- Etienne Pierre-Doray
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. |
Sean Maher (Gerrit)
Sean Maher added 2 comments![]( "Open in Gerrit")
Joe MasonLGTM if the CountResidentBytesInSharedMemory are removed (I haven't reviewed them).
And I see I'm not OWNER of that file anyway, so if Etienne reviews and approves those changes, go ahead and commit.
I'll have him review the follow-up, good call on moving the changes out.
etiennep@, can you PTAL @ the process memory dump changes?
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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 1 comment![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Code-Review | +1 |
1 comment
Related details
- Sean Maher
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
// Returns the number of bytes in a kernel memory page. Some platforms may
// have a different value for kernel page sizes from user page sizes. It is
// important to use kernel memory page sizes for resident bytes calculation.
// In most cases, the two are the same.
static size_t GetSystemPageSize();
// Returns the total bytes resident for a virtual address range, with given
// |start_address| and |mapped_size|. |mapped_size| is specified in bytes. The
// value returned is valid only if the given range is currently mmapped by the
// process. Works with exact non-page-aligned boundaries, but precisely
// page-aligned boundaries are performance-ideal. The returned value will
// never exceed |mapped_size|.
static std::optional<size_t> CountResidentBytes(void* start_address,
size_t mapped_size);
// The same as above, but the given mapped range should belong to the
// shared_memory's mapped region. The |start_address| must be page-aligned.
// TODO: let CountResidentBytesInSharedMemory support unaligned addresses
static std::optional<size_t> CountResidentBytesInSharedMemory(
void* start_address,
size_t mapped_size);Nit (feel free to ignore or punt): I feel like this has no place in base/trace_event/ and base/process/process_metrics.h would be the right place for these.
Related details
- Sean Maher
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. |
Sean Maher (Gerrit)
Sean Maher added 1 comment![]( "Open in Gerrit")
- 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. |