Closed Bug 1038888 Opened 10 years ago Closed 10 years ago

Extend system memory reporter to report ion memory

Categories

(Core :: XPCOM, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34
blocking-b2g 2.0+
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: erahm, Assigned: erahm)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(3 files)

Bug 948204 notes:
Note #1: Several vendors designed their own alternatives to pmem, which appear to be being obsoleted in favor of a new interface called ION — http://lwn.net/Articles/480055/ — so supporting pmem isn't the end of this (but that can be a separate bug).

Flame is is now using ION rather than pmem, we should report it.
Whiteboard: [MemShrink] → [MemShrink:P2]
This simply moves the RAII helpers to the beginning of the file, marks them as stack-only, and adds null checking.
Attachment #8461171 - Flags: review?(n.nethercote)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
This adds an ION system memory reporter. It parses the ion/iommu file and breaks out allocations per-process. If this is not an ION system it will silently move on.

Example output:

11.96 MB (100.0%) -- ion-memory
├───5.63 MB (47.04%) ── Homescreen (pid=2080) [20]
├───3.16 MB (26.46%) ── b2g (pid=1921) [2]
├───1.58 MB (13.23%) ── fd900000.qcom,mdss_mdp (pid=1)
├───1.58 MB (13.23%) ── mdss_fb0 (pid=1927)
└───0.00 MB (00.03%) ── adsprpc-smd (pid=1)
Attachment #8461174 - Flags: review?(n.nethercote)
Comment on attachment 8461171 [details] [diff] [review]
Part 1: Move RAII helpers to beginning of file

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

::: xpcom/base/SystemMemoryReporter.cpp
@@ +47,5 @@
>  #endif
>  
> +/**
> + * RAII helper that will close an open DIR handle.
> + */

Not sure if these comments are needed, but I guess they don't hurt.
Attachment #8461171 - Flags: review?(n.nethercote) → review+
Comment on attachment 8461174 [details] [diff] [review]
Part 2: Extend system memory reporter to report ion memory

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

Thanks!

::: xpcom/base/SystemMemoryReporter.cpp
@@ +735,5 @@
> +    // For our purposes we only care about the memory usage portion. The term
> +    // "orphaned" is misleading, it appears that every allocation not by the
> +    // first process is considered orphaned on FxOS devices.
> +
> +    // The first three fields interest us. We limit client names to 63

This is a lovely comment.

By "memory usage portion", I guess you mean the "orphaned allocations" section? Might as well say that directly. (Looking again, I think I'm wrong about that.)

And by "first three fields" I guess you mean e.g. "Homescreen", "24100" and "294912"... can you specifically mention what each field means? I had to look further down to the sscanf() call to understand exactly what fields 2 and 3 represented. (Oh, now I see the client/pid/size title line at the top, but it wasn't clear that applied to the orphaned section, probably because the orphaned lines have two extra unnamed fields.)

Anyway, judging by my comments above, the format of this file is confusing, and so a little more explanation will hopefully clear up the confusions.

@@ +761,5 @@
> +    uint32_t pid;
> +    uint64_t size;
> +
> +    size_t orphanedTotals = 0;
> +    size_t totals = 0;

These aren't used meaningfully, AFAICT.
Attachment #8461174 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #4)
> Comment on attachment 8461174 [details] [diff] [review]
> Part 2: Extend system memory reporter to report ion memory
> 
> Review of attachment 8461174 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: xpcom/base/SystemMemoryReporter.cpp
> @@ +735,5 @@
> > +    // For our purposes we only care about the memory usage portion. The term
> > +    // "orphaned" is misleading, it appears that every allocation not by the
> > +    // first process is considered orphaned on FxOS devices.
> > +
> > +    // The first three fields interest us. We limit client names to 63
> 
> By "memory usage portion", I guess you mean the "orphaned allocations"
> section? Might as well say that directly. (Looking again, I think I'm wrong
> about that.)

I'll reword, I mean the portion of the file I described above. There's more at the end of the file that we ignore.

> And by "first three fields" I guess you mean e.g. "Homescreen", "24100" and
> "294912"... can you specifically mention what each field means?

I'll explicitly list the field names, purpose, and type.

> @@ +761,5 @@
> > +    uint32_t pid;
> > +    uint64_t size;
> > +
> > +    size_t orphanedTotals = 0;
> > +    size_t totals = 0;
> 
> These aren't used meaningfully, AFAICT.

Leftover from debug testing, I'll remove them.
[Blocking Requested - why for this release]: Nom'ing for 2.0+, this should be useful for identifying and working on system memory regressions.
blocking-b2g: --- → 2.0?
https://hg.mozilla.org/mozilla-central/rev/eeccb95d2e83
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
QA Whiteboard: [qa-]
B2G 2.0 patch that will apply cleanly over the kgsl patch from bug 1038921.
Attachment #8462813 - Flags: review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: