Fix WinHeap accounting for large allocations [chromium/src : main]
José Dapena Paz (Gerrit)
José Dapena Paz has uploaded the change for review![]( "Open in Gerrit")
Commit message
Fix WinHeap accounting for large allocations
HeapWalk reports normal heap regions as PROCESS_HEAP_REGION entries, but
large allocations appear only as orphan busy entries outside any region.
Without tracking those, the WinHeap dump under-reports committed memory
in memory-infra traces.
Count busy entries that fall outside any region as committed memory.
Change diff
Change information
- M base/trace_event/malloc_dump_provider.cc
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
José Dapena Paz (Gerrit)
José Dapena Paz added 1 comment![]( "Open in Gerrit")
This is the first in a series of changes to the memory accounting, that intends to improve and unify how the malloc/ section in memory infra captures is represented.
Related details
- Mikhail Khokhlov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikhail Khokhlov (Gerrit)
Mikhail Khokhlov added 1 comment![]( "Open in Gerrit")
ssid@ PTAL, since this is memory-infra related.
Related details
- José Dapena Paz
- Siddhartha S
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siddhartha S (Gerrit)
Siddhartha S added 2 comments![]( "Open in Gerrit")
void WinHeapMemoryDumpImpl(WinHeapInfo* crt_heap_info) {can we test this in unittests. can they call HeapWalk? can we add some sanity checks for range of numbers we should see..
if (entry_addr < last_region_start || entry_addr >= last_region_end) {this code depends on the ordering guarantees of the HeapWalk, maybe add a comment to the documentation that provides this guarantee
Related details
- José Dapena Paz
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. |
Will Harris (Gerrit)
Will Harris added 1 comment![]( "Open in Gerrit")
This is the first in a series of changes to the memory accounting, that intends to improve and unify how the malloc/ section in memory infra captures is represented.
If this is a series of CLs can you create a task in the issue tracker representing the goal and the approach so the CLs can be tagged to the bug. Thanks.
Related details
- José Dapena Paz
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. |
José Dapena Paz (Gerrit)
José Dapena Paz added 3 comments![]( "Open in Gerrit")
This is the first in a series of changes to the memory accounting, that intends to improve and unify how the malloc/ section in memory infra captures is represented.
If this is a series of CLs can you create a task in the issue tracker representing the goal and the approach so the CLs can be tagged to the bug. Thanks.
I created independent tickets for each bug, and each of the functional changes, as all were surfaced when trying to improve the malloc reported information in Linux and Windows, but there is not strict relation among them.
Bugs:
- Renamed [Fix WinHeap committed/virtual accounting for orphan busy regions](https://issues.chromium.org/483623915). This commit fixes this.
- Created [PartitionAlloc malloc dump virtual_size uses resident bytes](https://issues.chromium.org/510830984).
- Created [Fix sys_malloc dump: waste and fragmentation collapse to zero on the mallinfo path](https://issues.chromium.org/510816358).
Then the features:
- Created [MemoryInfra: uniformize malloc/allocated_objects breakdown across malloc backends](https://issues.chromium.org/510995905).
- Renamed [MemoryInfra: uniformize virtual_size and virtual_committed_size reporting across malloc backends](https://issues.chromium.org/483623913).
- Created [MemoryInfra: uniformize waste and fragmentation reporting across malloc backends](https://issues.chromium.org/510816359).
can we test this in unittests. can they call HeapWalk? can we add some sanity checks for range of numbers we should see..
Adding unit tests (with a small refactoring to be able to call the win heap dump from the test).
if (entry_addr < last_region_start || entry_addr >= last_region_end) {this code depends on the ordering guarantees of the HeapWalk, maybe add a comment to the documentation that provides this guarantee
I will add a comment referring to the SDK document explaining this.
Related details
- Siddhartha S
- Will Harris
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris added 4 comments![]( "Open in Gerrit")
this is cool I did not know about this behavior of heap where the values were optional. this seems like a very reasonable approach to fix this issue. thank you!
what are these data used for - is this going to change metrics in any way?
BASE_EXPORT void WinHeapMemoryDumpImpl(void* heap, WinHeapInfo* info);`info` is a non-optional out param, so please use a ref here.
reinterpret_cast<HANDLE>(_get_heap_handle()), &main_heap_info);this is passing a HANDLE and yet the function takes a void* - can you fix?
info->committed_size += heap_entry.cbData + heap_entry.cbOverhead;cbData is DWORD 32-bit - should this be promoted to a `size_t` before the addition, so very large `cbData` sizes near the 32-bit boundary can't overflow?
e.g.
`static_cast<size_t>(heap_entry.cbData)`
Related details
- José Dapena Paz
- Siddhartha S
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. |
Siddhartha S (Gerrit)
Siddhartha S added 2 comments![]( "Open in Gerrit")
Moving Will to reviewer.
this is cool I did not know about this behavior of heap where the values were optional. this seems like a very reasonable approach to fix this issue. thank you!
what are these data used for - is this going to change metrics in any way?
I believe the MemoryDumpLevelOfDetail::kDetailed should only be triggered on local tracing. so, this change should not impact chrome metrics, including any malloc related uma counters.
Related details
- José Dapena Paz
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. |
José Dapena Paz (Gerrit)
José Dapena Paz added 3 comments![]( "Open in Gerrit")
BASE_EXPORT void WinHeapMemoryDumpImpl(void* heap, WinHeapInfo* info);`info` is a non-optional out param, so please use a ref here.
As we always do the same:
```
WinHeapInfo info;
WinHeapMemoryDumpImpl(heap, &info);
```
I will move to have a static in `WinHeapInfo` for creating it from the handle:
```
WinHeapInfo WinHeapInfo::FromHandle(heap);
```
reinterpret_cast<HANDLE>(_get_heap_handle()), &main_heap_info);this is passing a HANDLE and yet the function takes a void* - can you fix?
Done
info->committed_size += heap_entry.cbData + heap_entry.cbOverhead;cbData is DWORD 32-bit - should this be promoted to a `size_t` before the addition, so very large `cbData` sizes near the 32-bit boundary can't overflow?
e.g.
`static_cast<size_t>(heap_entry.cbData)`
- Will Harris
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. |
Will Harris (Gerrit)
Will Harris added 1 comment![]( "Open in Gerrit")
static BASE_EXPORT WinHeapInfo FromHandle(void* heap);if exposed for testing can you add `ForTesting` on the method name, so the static checks assert it's only called in tests.
to support production code, add a new non-test only API that calls `_get_heap_handle`.
Related details
- José Dapena Paz
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. |
José Dapena Paz (Gerrit)
José Dapena Paz added 2 comments![]( "Open in Gerrit")
if exposed for testing can you add `ForTesting` on the method name, so the static checks assert it's only called in tests.
to support production code, add a new non-test only API that calls `_get_heap_handle`.
Done
BASE_EXPORT void WinHeapMemoryDumpImpl(void* heap, WinHeapInfo* info);José Dapena Paz`info` is a non-optional out param, so please use a ref here.
As we always do the same:
```
WinHeapInfo info;
WinHeapMemoryDumpImpl(heap, &info);
```I will move to have a static in `WinHeapInfo` for creating it from the handle:
```
WinHeapInfo WinHeapInfo::FromHandle(heap);
```
The approach for returning the value is done. And following the proposal below, the only public API is for testing. The internal API works already with HANDLE.
Related details
- Will Harris
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. |
Will Harris (Gerrit)
Will Harris voted and added 2 comments![]( "Open in Gerrit")
Votes added by Will Harris
| Code-Review | +1 |
2 comments
internal::WinHeapInfo WinHeapInfoFromCRTHeap() {nit: no need for this function, you can call directly from production code
but no problem keeping it either.
Related details
- José Dapena Paz
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. |
José Dapena Paz (Gerrit)
José Dapena Paz added 1 comment![]( "Open in Gerrit")
nit: no need for this function, you can call directly from production code
but no problem keeping it either.
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. |
José Dapena Paz (Gerrit)
José Dapena Paz added 1 comment![]( "Open in Gerrit")
this is cool I did not know about this behavior of heap where the values were optional. this seems like a very reasonable approach to fix this issue. thank you!
what are these data used for - is this going to change metrics in any way?
I believe the MemoryDumpLevelOfDetail::kDetailed should only be triggered on local tracing. so, this change should not impact chrome metrics, including any malloc related uma counters.
- Siddhartha S
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
José Dapena Paz (Gerrit)
José Dapena Paz added 1 comment![]( "Open in Gerrit")
- Etienne Pierre-Doray
- Siddhartha S
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siddhartha S (Gerrit)
Siddhartha S voted and added 1 comment![]( "Open in Gerrit")
Votes added by Siddhartha S
| Code-Review | +1 |
1 comment
sorry was ooo, lgtm stamp, since Will reviewed it in detail
Related details
- Etienne Pierre-Doray
- José Dapena Paz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
José Dapena Paz (Gerrit)
José Dapena Paz voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
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. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Fix WinHeap accounting for large allocations
HeapWalk reports normal heap regions as PROCESS_HEAP_REGION entries, but
large allocations appear only as orphan busy entries outside any region.
Without tracking those, the WinHeap dump under-reports committed memory
in memory-infra traces.
Count busy entries that fall outside any region as committed memory.
- M base/BUILD.gn
- M base/trace_event/malloc_dump_provider.cc
- M base/trace_event/malloc_dump_provider.h
- A base/trace_event/malloc_dump_provider_unittest.cc
Code-Review: +1 by Will Harris, +1 by Siddhartha S
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |