Closed Bug 1151597 Opened 5 years ago Closed 5 years ago

Stream e10s memory reports over IPC instead of sending one big array.

Categories

(Toolkit :: about:memory, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
e10s + ---
firefox40 --- fixed
b2g-master --- fixed

People

(Reporter: jld, Assigned: jld)

References

Details

(Whiteboard: [MemShrink:P1])

Attachments

(3 files, 4 obsolete files)

+++ This bug was initially created as a clone of Bug #1145005 +++

Quoting bug 1145005 comment #1:
> There is definitely room for improvement here — PMemoryReportRequest sends
> the child's entire memory report as a single array rather than streaming it.

One thing to be careful of: if I correctly understand enough of how IPC works, the parent process has to finish its report before messaging the children to start theirs, because the messages from the children are handled on the parent's main thread, and if it's busy doing its own report and not handling messages, they'll be queued in memory and this won't actually help.  This could also be an issue if the children can (collectively) send messages faster than the parent can translate them to JSON and gzip them.

The JSON serialization appears to already be streaming, so that shouldn't be a problem, and the core memory reporting is already defined in terms of a callback invoked on every leaf measurement.
Not strictly part of this bug, but I wound up doing it anyway, and thought it ought to be separated from the rest of the IPDL change.  Copying the justification from the patch commit message: There's no reason for the child to send the generation number back to the parent when the parent already had it; the IPDL actor abstraction will ensure that the binding between parent-side state and child-side state is maintained.
Attachment #8597473 - Flags: review?(erahm)
The important part.  Also fixes bug 1005154.  (The parent still does its reporting in parallel with the children, here.  That's the next patch....)
Attachment #8597474 - Flags: review?(erahm)
This might be a little easier to read in some places.
Comment on attachment 8597473 [details] [diff] [review]
Step 0: Move IPC memory report generation number to parent-side actor

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

r=me

::: dom/ipc/ContentParent.cpp
@@ +413,5 @@
>      }
>  };
>  
> +MemoryReportRequestParent::MemoryReportRequestParent(uint32_t aGeneration)
> +: mGeneration(aGeneration)

nit: indentation
Attachment #8597473 - Flags: review?(erahm) → review+
Comment on attachment 8597474 [details] [diff] [review]
Step 1: Change memory reporting IPC to send one report per message.

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

r=me
Attachment #8597474 - Flags: review?(erahm) → review+
Comment on attachment 8597476 [details] [diff] [review]
Step 2: Don't start child process memory reports until parent is finished.

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

r=me Nice to see this simplifies things a bit as well.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +1386,5 @@
> +    // after the parent report so that the parent's main thread will
> +    // be free to process the child reports, instead of causing them
> +    // to be buffered and consume (possibly scarce) memory.
> +    nsCOMPtr<nsITimer> timer = do_CreateInstance(NS_TIMER_CONTRACTID);
> +    // Don't use NS_ENSURE_* here; must ensure the report is finished.

nit: Maybe rephrase this, 'Don't use NS_ENSURE_* here; must ensure' is a bit confusing.
Attachment #8597476 - Flags: review?(erahm) → review+
Carrying over r+.
Attachment #8597474 - Attachment is obsolete: true
Attachment #8597475 - Attachment is obsolete: true
Attachment #8598307 - Flags: review+
Attachment #8598307 - Attachment description: bug1151597-p1-no-array-hg1.diff → Step 1: Change memory reporting IPC to send one report per message. [v2]
Attachment #8597476 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.