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

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Step 0: Move IPC memory report generation number to parent-side actor

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.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Step 1: Change memory reporting IPC to send one report per message.

The important part.  Also fixes bug 1005154.  (The parent still does its reporting in parallel with the children, here.  That's the next patch....)

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: (Step 1 without whitespace-only changes.)

This might be a little easier to read in some places.

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Step 2: Don't start child process memory reports until parent is finished.

Comment 5

[Eric Rahm [:erahm]]

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

Comment 6

[Eric Rahm [:erahm]]

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

Comment 7

[Eric Rahm [:erahm]]

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.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Step 0: Move IPC memory report generation number to parent-side actor [v2]

Carrying over r+.

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Step 1: Change memory reporting IPC to send one report per message. [v2]

Carrying over r+.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Step 2: Don't start child process memory reports until parent is finished. [v2]

Carrying over r+.

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17a53c6f89a4

Comment 12

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b686afd53196
https://hg.mozilla.org/integration/mozilla-inbound/rev/57ffca487e0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/338486ce551e

Comment 13

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/mozilla-central/rev/b686afd53196
https://hg.mozilla.org/mozilla-central/rev/57ffca487e0a
https://hg.mozilla.org/mozilla-central/rev/338486ce551e