927740 - Fix up all the memory management observations

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Nicholas Nethercote [inactive]]

In about:memory, the "GC" button triggers GC in the current process and the child processes.  Likewise for the "CC" button.  But the "Minimize memory usage" button only does stuff in the current process.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Make about:memory's "Minimize Memory Usage" button affect child processes, like the "GC" and "CC" buttons.

Turns out we already have the machinery in place for sending MMU requests to
children (i.e SendMinimizeMemoryUsage() and RecvMinimizeMemoryUsage()).  So I
just had to hook up an observer.

Comment 2

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Comment on attachment 832908 [details] [diff] [review]
Make about:memory's "Minimize Memory Usage" button affect child processes, like the "GC" and "CC" buttons.

Looks fine to me, but I don't understand all the machinery here, so I'm going to pass the buck.  For instance, why is there stuff in RuntimeService for child-gc-request but not child-cc-request?
  http://mxr.mozilla.org/mozilla-central/ident?i=GC_REQUEST_OBSERVER_TOPIC

Comment 3

[Nicholas Nethercote [inactive]]

> why is there stuff in RuntimeService
> for child-gc-request but not child-cc-request?

Interesting question.  It looks like workers won't get CC'd when a CC is requested.

There are some other inconsistencies in this stuff, e.g. when a child gets a CC request it actually does GC+CC.

But these are both orthogonal to this patch, and make sense to do as follow-up patches.

Comment 4

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Sure, my point was just that I wasn't sure what your patch needed, as it doesn't have the "extra child-gc-request stuff" as far as I could see.

Comment 5

[Nicholas Nethercote [inactive]]

Hmm, it's an interesting question if nsMemoryReporterManager::MinimizeMemoryUsage() should just work with the current process or with all child processes too.  My patch does the latter, but I could instead leave it as is and instead send the "minimize-memory-pressure" event from aboutMemory.js, as is done for the GC and CC events.

Comment 6

[Nicholas Nethercote [inactive]]

I'm broadening this bug's scope to cover all the inconsistencies in how these things are handled.

Comment 7

[Nicholas Nethercote [inactive]]

Here's what happens with the three about:memory buttons.  Problems are numbered.

"GC" button:
- parent process calls GCForReason() on the main JSRuntime
- triggers "child-gc-request"
  - RuntimeService observes it, and all workers (parent and child) call 
    GCForReason (via GarbageCollectRunnable)
  - ContentParent observes it; each child process is triggered and calls
    GarbageCollectNow() on main JSRuntime
    - nothing happens with child process workers [1]

"CC" button:
- parent process CycleCollectNow() on the main JSRuntime
- triggers "child-cc-request"
  - RuntimeService doesn't observe it [2]
  - ContentParent observes it; each child process is triggered and calls
    GarbageCollectNow() *and* CycleCollectNow() [3] on the main runtime
    - nothing happens with child process workers [4]

"MMU" button:
- triggers three "memory-pressure" events and a "after-minimize-memory-usage"
  event
  - nsJSEnvironmentObserver observes it, and calls
    GarbageCollectNow(ShrinkingGC) and CycleCollectNow()
  - RuntimeService observes it, and all workers (parent and child) call
    ShrinkingGC(), but no CC [5]
  - ContentParent doesn't observe it [6] -- nothing done with child processes

Here's how I think we can fix the problems.
- rebroadcast "child-{gc,cc}-request" and "child-cc-request" in child
  processes, so workers will GC/CC [1] [4]
- make RuntimeService observe "child-cc-request" [2]
- make "child-cc-request" only trigger CC, not GC, in child processes [3]
- make RuntimeService do CC on "memory-pressure" [5]
- add "child-mmu-request", broadcast it on "MMU" button press, and then
  make it trigger MinimizeMemoryUsage() in child processes accordingly [6]

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Fix observation of GC and CC requests by child processes.

This patch fixes problems [1], [4] and [3].  Actually, [1] and [4] seem to be
non-problems currently, because child processes don't appear to use workers?
It still seems worth rebroadcasting the notifications in case that changes in
the future.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Make about:memory's "Minimize Memory Usage" button affect child processes, like the "GC" and "CC" buttons.

This patch fixes problem [6].

Note that child-mmu-request is only observed by child processes, whereas
child-{gc,cc}-request is observed by child processes and workers in the parent
process.  (Workers instead observe the memory-pressure events that
MinimizeMemoryUsage() triggers.)

So I wonder if we should rename them as "child-and-worker-{gc,cc}-request" to
clarify things.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Make workers respond to "child-cc-request" notifications.

This patch fixes problem [2].

The dummy arg to CycleCollect() sucks, but I can't see how to modify
BROADCAST_ALL_WORKERS to handle zero-arg functions.

It'd be nice to have tests for all this stuff, but I don't know how they would
work.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Make workers CC on memory-pressure.

This patch fixes problem [5].

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8335109 [details] [diff] [review]
(part 3) - Make workers respond to "child-cc-request" notifications.

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

::: dom/workers/RuntimeService.cpp
@@ +2215,5 @@
>      Cleanup();
>      return NS_OK;
>    }
>    if (!strcmp(aTopic, GC_REQUEST_OBSERVER_TOPIC)) {
> +    GarbageCollectAllWorkers(/* shrinking = */ false);

njn++

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8335106 [details] [diff] [review]
(part 1) - Fix observation of GC and CC requests by child processes.

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

::: dom/ipc/ContentChild.cpp
@@ -1284,5 @@
>  
>  bool
>  ContentChild::RecvCycleCollect()
>  {
> -    nsJSContext::GarbageCollectNow(JS::gcreason::DOM_IPC);

Why did you remove this?

Comment 14

[Nicholas Nethercote [inactive]]

> >  bool
> >  ContentChild::RecvCycleCollect()
> >  {
> > -    nsJSContext::GarbageCollectNow(JS::gcreason::DOM_IPC);
> 
> Why did you remove this?

That's problem [3] -- prior to my patch, everywhere else the "CC" button just triggers cycle collection, but in the child it triggers GC as well.

Comment 15

[Nicholas Nethercote [inactive]]

On memory pressure, should workers CC after GC?

Comment 16

[Nicholas Nethercote [inactive]]

> On memory pressure, should workers CC after GC?

Oh, that's what patch 4 does.  Never mind.

Comment 17

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24e2453b9e53
https://hg.mozilla.org/integration/mozilla-inbound/rev/14a0fe515c9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eea4558dff5
https://hg.mozilla.org/integration/mozilla-inbound/rev/03171880d9f4

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/24e2453b9e53
https://hg.mozilla.org/mozilla-central/rev/14a0fe515c9c
https://hg.mozilla.org/mozilla-central/rev/4eea4558dff5
https://hg.mozilla.org/mozilla-central/rev/03171880d9f4