Report memory regions by status bits

RESOLVED FIXED in Firefox 37

Status

()

Toolkit
about:memory
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dmajor, Assigned: njn)

Tracking

unspecified
mozilla39
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed, firefox39 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
I have a modified version of breakpad's minidump-memorylist tool that breaks down address space by the page status and protection bits. It has been very useful for issues like bug 1062065 and bug 1128999.

We can build this into about:memory pretty easily. The memory reporters already do an address space walk for the vsize-max-contiguous calculation: https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsMemoryReporterManager.cpp#509
(Reporter)

Comment 1

2 years ago
Created attachment 8565708 [details] [diff] [review]
patch on top of minidump-memorylist
(Reporter)

Comment 2

2 years ago
So far RESV and GFX have been the most useful. FREE and HEAP are probably covered by existing reporters. The remaining categories may or may not be useful someday.
(Reporter)

Comment 3

2 years ago
As an additional project we might also want to take the "Tiny" and "Misaligned" numbers from attachment 8428481 [details] [diff] [review] (of bug 1001760).
(Reporter)

Comment 4

2 years ago
Nick, could your team take it from here? Just need to take the logic from the patch and make it about-memory-esque.
Flags: needinfo?(n.nethercote)
(Assignee)

Updated

2 years ago
Assignee: nobody → n.nethercote
Flags: needinfo?(n.nethercote)
(Assignee)

Comment 5

2 years ago
Created attachment 8566924 [details] [diff] [review]
Add WindowsAddressSpaceReporter. code=njn,dmajor

Sample output:

> 2,147,418,112 B (100.0%) -- address-space
> +--1,662,676,992 B (77.43%) -- free [229]
> +----299,868,160 B (13.96%) -- commit
> |    +--142,098,432 B (06.62%) -- image
> |    |  +---94,035,968 B (04.38%) -- execute-read [107]
> |    |  +---44,826,624 B (02.09%) -- readonly [245]
> |    |  +----1,736,704 B (00.08%) -- writecopy [62]
> |    |  +----1,499,136 B (00.07%) -- readwrite [151]
> |    +--127,987,712 B (05.96%) -- private
> |    |  +--122,867,712 B (05.72%) -- readwrite [224]
> |    |  +----2,977,792 B (00.14%) -- readwrite (stack) [128]
> |    |  +----1,310,720 B (00.06%) -- readwrite+guard [128]
> |    |  +------729,088 B (00.03%) -- execute-readwrite [13]
> |    |  +-------53,248 B (00.00%) -- execute-read [11]
> |    |  +-------49,152 B (00.00%) -- readonly [12]
> |    +---29,782,016 B (01.39%) -- mapped
> |        +--18,513,920 B (00.86%) -- readwrite [31]
> |        +--11,268,096 B (00.52%) -- readonly [25]
> +----184,872,960 B (08.61%) -- reserved
>      +--161,124,352 B (07.50%) -- private [177]
>      +---23,474,176 B (01.09%) -- mapped [5]
>      +------274,432 B (00.01%) -- image [6]

The numbers in square brackets indicate how many ranges had those attributes.
It's reassuring that the number of "readwrite (stack)" ranges matches the
number of "readwrite+guard" ranges. It's also reassuring that the total is a
smidge under 2 GiB.
Attachment #8566924 - Flags: review?(dmajor)
(Assignee)

Comment 6

2 years ago
glandium pointed out that "free" will dominate on 64-bit...

> 134,217,727.94 MB (100.0%) -- address-space
> +--134,217,127.41 MB (100.0%) -- free [208]
> +----------600.53 MB (00.00%) ++ (2 tiny)

128 TiB! dmajor, any thoughts about this? Maybe it's not a problem.
(Reporter)

Comment 7

2 years ago
> glandium pointed out that "free" will dominate on 64-bit...
Amusing :) but IMO it's not really a problem.
(Reporter)

Comment 8

2 years ago
Comment on attachment 8566924 [details] [diff] [review]
Add WindowsAddressSpaceReporter. code=njn,dmajor

Review of attachment 8566924 [details] [diff] [review]:
-----------------------------------------------------------------

I really like this approach! Thanks for working on this.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +680,5 @@
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return rv;
> +      }
> +
> +      SIZE_T lastAddress = currentAddress;

currentAddress is a lowercase size_t so I guess lastAddress can be lowercase too.

@@ +689,5 @@
> +        break;
> +      }
> +    }
> +
> +#undef REPORT

What's this for?
Attachment #8566924 - Flags: review?(dmajor) → review+
(Assignee)

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0e55dfe6955
https://hg.mozilla.org/mozilla-central/rev/e0e55dfe6955
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment on attachment 8566924 [details] [diff] [review]
Add WindowsAddressSpaceReporter. code=njn,dmajor

Requesting uplift to 38 and 37. We think this might help us diagnose remaining reported memory issues with the MSE feature.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: weaker testing, less information available during bug reproduction.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This only affects about:memory and has had two weeks to settle on m-c, so risk should be acceptable.
[String/UUID change made/needed]: None.
Attachment #8566924 - Flags: approval-mozilla-beta?
Attachment #8566924 - Flags: approval-mozilla-aurora?
Attachment #8566924 - Flags: approval-mozilla-beta?
Attachment #8566924 - Flags: approval-mozilla-beta+
Attachment #8566924 - Flags: approval-mozilla-aurora?
Attachment #8566924 - Flags: approval-mozilla-aurora+
status-firefox37: --- → affected
status-firefox38: --- → affected
https://hg.mozilla.org/releases/mozilla-beta/rev/20306323469e
status-firefox37: affected → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/04c022de147f
status-firefox38: affected → fixed
You need to log in before you can comment on or make changes to this bug.