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. |