Last Comment Bug 754495 - represent an entire compartment as a single CC node when tearing down a page
: represent an entire compartment as a single CC node when tearing down a page
Status: RESOLVED FIXED
[Snappy]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla16
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
Depends on: 742841 757884 763776
Blocks: 741417 k9o-perf 763730
  Show dependency treegraph
 
Reported: 2012-05-11 16:45 PDT by Andrew McCreight [:mccr8]
Modified: 2012-07-05 02:52 PDT (History)
22 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
compartment merging (55.80 KB, application/pdf)
2012-05-11 16:45 PDT, Andrew McCreight [:mccr8]
no flags Details
WIP, doesn't crash immediately (14.06 KB, patch)
2012-05-14 10:18 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
with compartment merging (98.04 KB, application/pdf)
2012-05-14 10:37 PDT, Andrew McCreight [:mccr8]
no flags Details
revised compartment merging patch (26.02 KB, patch)
2012-05-16 16:59 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 2: add simple heuristic for scheduling compartmental merging CCs (5.59 KB, patch)
2012-05-16 17:09 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 1: add JS hooks for scanning CC things in a compartment (4.68 KB, patch)
2012-05-18 14:00 PDT, Andrew McCreight [:mccr8]
wmccloskey: feedback+
Details | Diff | Splinter Review
part 2: implement CompartmentParticipant, add hooks for it in CC (22.40 KB, patch)
2012-05-18 14:06 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 3: schedule merging CC after killing windows (5.52 KB, patch)
2012-05-18 14:08 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 1: add JS hooks for scanning CC things in a compartment (4.36 KB, patch)
2012-06-25 13:30 PDT, Andrew McCreight [:mccr8]
wmccloskey: review+
Details | Diff | Splinter Review
part 2: add flag to control JS CC traversal behavior (3.93 KB, patch)
2012-06-26 08:46 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review
part 3: add shim to NoteJSChild to allow it to be reused (2.06 KB, patch)
2012-06-26 08:48 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review
part 4: define CompartmentParticipant (5.60 KB, patch)
2012-06-26 09:45 PDT, Andrew McCreight [:mccr8]
wmccloskey: review+
bugs: review+
Details | Diff | Splinter Review
part 5: add support for JSCompartment merging to the CC (15.02 KB, patch)
2012-06-26 09:51 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review
part 6: indicate in error console if we did a merging CC (3.57 KB, patch)
2012-06-26 09:52 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review
part 7: indicate if CycleCollectNow is forced (3.41 KB, patch)
2012-06-26 09:56 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review
part 8: do a merging CC when there's a gray global (1.54 KB, patch)
2012-06-26 10:14 PDT, Andrew McCreight [:mccr8]
bugs: review+
mrbkap: review+
Details | Diff | Splinter Review
part 9: don't merge too much (1.41 KB, patch)
2012-06-26 11:55 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review
(folded patch for testing) (35.91 KB, patch)
2012-06-26 14:29 PDT, Andrew McCreight [:mccr8]
continuation: checkin-
Details | Diff | Splinter Review
part 10: followup to fix assertion (1.17 KB, patch)
2012-06-29 09:05 PDT, Andrew McCreight [:mccr8]
Ms2ger: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2012-05-11 16:45:24 PDT
Created attachment 623363 [details]
compartment merging

Here's another idea for reducing CC pauses when tearing down pages, which is a less radical version of bug 751283: if we somehow decide we're killing a particular compartment (perhaps by doing something similar to bug 695480), we represent the entire compartment as a single node in the CC graph.  We treat pointers to objects in that compartment as pointers to the compartment.  We implement a special JSCompartment participant.

I'm hoping this can be implemented more easily than some of my other ideas for speeding CC, and that it will help in the worst cases.

A) Out edges

Instead of tracing through individual objects in a compartment, we directly scan the compartment for out edges.

There are two kinds of out edges of a compartment.

1) Cross-compartment wrappers (CCW).  Each compartment keeps a table of wrappers, so we can NoteJSChild all of them.  Any wrapper that is the target of a marked object must itself also be marked, and thus won't be added to the CC graph.

2) C++ pointers.  For this, we call nsXPConnect::Traverse on all gray objects in the compartment, forwarding the NoteXPCOMChildren we get, and ignoring the rest.

Each compartment is treated as unmarked.  This is only okay for non-WANT_ALL_TRACES CCs, but we avoid that problem by not doing compartment blobbing in W_A_T CCs.  In essence, we are only dealing with the portion of the compartment that is marked gray.  Unlinking a compartment, like unlinking a JS object, doesn't do anything so this should be okay, as long as we respect the reachability relation for all paths we do take.  We don't add any edges from black objects to C++ objects, so they will miss a reference, and be kept alive.  For CCWs, if the source in our compartment is black, then the target in another compartment should also be black, and thus won't be added to the graph.

B) Performance

Scanning compartments in this way should hopefully be faster than tracing through it.
1) We don't ever have to examine non-objects (shapes, type objects, strings, etc.).
2) Locality should be better because we're tracing linearly through arenas instead of randomly, and intermixed with other things outside of the compartment.
3) There is less XPConnect-CC glue.

C) Experiment

I did a very crude experiment which suggests that in the case of closing a tab you can determine that a compartment is garbage without examining its interior geometry.  See the attached CC graph, which only includes the largest globs in the graph.

What I did here was modify the CC logging to print out the compartment of each JS object, then I merged together all JS things in a single compartment together in the graph.  The squares are JS things that are garbage.  There are two very large ones, one with 9933 objects in it, and another with 2472 objects.  In this particular case, it appears that these compartment blobs would not be kept alive by anything, even with this imprecise information about reachability.

Side node: the hexagon with 1517 nodes in it is a DOM.  Separately, I'd like to look into khuey's idea for merging together DOMs in the CC graph.  A DOM is strongly connected, so merging a DOM in that way is precise.
Comment 1 Andrew McCreight [:mccr8] 2012-05-14 10:18:08 PDT
Created attachment 623712 [details] [diff] [review]
WIP, doesn't crash immediately
Comment 2 Andrew McCreight [:mccr8] 2012-05-14 10:34:54 PDT
To my great shock, this seems to work reasonable well for at least a simple example, of closing a single tab with this page on it: https://wiki.mozilla.org/Performance:Leak_Tools#Cycle_collector_heap_dump

without merging:

CC(T+31.1) duration: 22ms, suspected: 66, visited: 2226 RCed and 10395 GCed, collected: 1678 RCed and 9954 GCed (11632 waiting for GC)
ForgetSkippable 5 times before CC, min: 0 ms, max: 5 ms, avg: 1 ms, total: 7 ms, removed: 1310


with merging:

CC(T+39.8) duration: 8ms, suspected: 79, visited: 2257 RCed and 34 GCed, collected: 1663 RCed and 4 GCed (1667 waiting for GC)
ForgetSkippable 6 times before CC, min: 0 ms, max: 3 ms, avg: 1 ms, total: 9 ms, removed: 1594

This is a debug build, so the timing may be suspect, but you can see that this got rid of 80% of the graph.  Traversing the compartment is probably going to be slower than an individual object, but it looks like it might be faster overall.
Comment 3 Andrew McCreight [:mccr8] 2012-05-14 10:37:40 PDT
Created attachment 623721 [details]
with compartment merging

This is the entire CC graph, with compartment merging.  I still use post-facto DOM merging, as in the other PDF file.  But you can see for the two blobs present in the normal graph (with 10000 and 2500 nodes, approx), they have been reduced to a single node.
Comment 4 Andrew McCreight [:mccr8] 2012-05-14 10:56:19 PDT
Here's another fun little comparison.  I opened 5 Tech Crunch tabs, then a new empty tab, then let things settle out, then I did "close all other tabs" on the empty tab, closing the 5 tech crunch tabs.

with merging:

CC(T+74.0) duration: 99ms, suspected: 1665, visited: 28869 RCed and 93 GCed, collected: 27962 RCed and 59 GCed (28021 waiting for GC)
ForgetSkippable 3 times before CC, min: 0 ms, max: 6 ms, avg: 4 ms, total: 13 ms, removed: 194

without merging:

CC(T+57.1) duration: 568ms, suspected: 1702, visited: 28911 RCed and 207627 GCed, collected: 28175 RCed and 207184 GCed (235359 waiting for GC)
ForgetSkippable 4 times before CC, min: 0 ms, max: 8 ms, avg: 4 ms, total: 19 ms, removed: 383
Comment 5 Andrew McCreight [:mccr8] 2012-05-15 17:06:50 PDT
My experimental patch above always merges compartments.  This is bad because it is possible to have one cycle through a compartment that is garbage while there's another that isn't, so merging will cause us to hold memory alive for longer.  It is also possible that if we have a single object in a compartment to be added to the CC graph, it will be faster to just add that object.

I need to decide when we want to enable compartmental merging.  I've been trying it out in nsGlobalWindow::FreeInnerObjects().  Generally, we want to enable it for a compartment that we're pretty sure is going to be freed.

We could enable compartment merging either globally or on a per-compartment basis.

There's also a matter of how long it should remain enabled.  If a compartment has been marked mostly black by CC optimizations, then we really want to keep merging it until there has been a GC.

When I was trying out a variant of my patch that set global merging for the first CC after a FreeInnerObjects(), I noticed a case that went:
FreeInnerObjects() -> merged CC that doesn't do anything -> GC -> big CC

So that's bad.  I could wait until there has been a GC, but in the modern era where the browser does compartmental GCs more often, that may not be sufficient.  We may want to do something like suspect a compartment, and then keep merging until after a GC has been done on that compartment.  I don't know how hard that is.

Anyways, as you can see there's a pretty huge state space here, and I'd appreciate any thoughts people may have.
Comment 6 Andrew McCreight [:mccr8] 2012-05-15 17:08:13 PDT
Also, FYI, a Linux64 Try run was green, except for some Moth tests that were crashing, probably due to my failure to support Weak Maps.  Olli tried out my patch, and it sounded like it was pretty leaky.  Leaks caused by this patch won't show up as shutdown leaks, because I think the browser kind of throws out compartments pretty aggressively at shutdown.
Comment 7 Olli Pettay [:smaug] 2012-05-16 00:39:55 PDT
I think the leaks I saw were somehow related to a_chrome_compartment<->another_chrome_compartment edges.
Comment 8 Andrew McCreight [:mccr8] 2012-05-16 07:30:49 PDT
(In reply to Olli Pettay [:smaug] from comment #7)
> I think the leaks I saw were somehow related to
> a_chrome_compartment<->another_chrome_compartment edges.

It might be a good idea to only merge content compartments at first, as those are probably larger and more common to tear down.  Of course, we still want to not merge them all the time.
Comment 9 Andrew McCreight [:mccr8] 2012-05-16 15:05:24 PDT
I think the strategy I'm going to go with is a global setting in a CC to merge compartments or not.  The extra complexity from a more fine grained enabling of merging doesn't seem to buy you anything, and this will let me hoist the decision about merging or not to nsJSEnvironment, which knows when GCs are scheduled.

I did some timing of how long the compartment scanning was taking, and while it takes a decent chunk of time, it doesn't seem to take more than 10% of the CC time, so it isn't really worth trying to flip it on or off for individual compartments to try to eke out speed improvements in the case where only a few objects in a compartment would be visited.
Comment 10 Andrew McCreight [:mccr8] 2012-05-16 16:59:55 PDT
Created attachment 624600 [details] [diff] [review]
revised compartment merging patch

This patch is similar to the previous version, but it adds a new argument to the cycle collector that controls whether compartments are merged, to allow us to control scheduling merged compartments from nsJSEnvironment.
Comment 11 Andrew McCreight [:mccr8] 2012-05-16 17:09:37 PDT
Created attachment 624604 [details] [diff] [review]
part 2: add simple heuristic for scheduling compartmental merging CCs

Here's a rough attempt at a fairly simple heuristic for controlling compartmental merging for CCs.

This adds a function |nsJSContext::PrepareForBigCC()| that is supposed to be called in places where you suspect a compartment is going to begin the process of being torn down.  Right now, this is only called in nsGlobalWindow::FreeInnerObjects().  When that function is called, it sets a flag |GotPrepForBigCC|.

We don't necessarily do a merging CC at the next CC: we assume the compartment being torn down was subject to extensive CC optimizations, and thus likely has been marked black and won't truly be ready for being torn down until the GC can be run again.  At the start of the next GC where the flag has been set, we clear that flag and set another flag, NextCCIsBig.  (Note that due to incremental GC, we want to do this at the start of the GC and not the end: any compartment that starts the tear down process during the GC won't necessarily get its black bits cleared during this GC.)

At the start of a CC, if NextCCIsBig is set, we clear it and do a merging CC.

Having two flags allows us to cover the case where we signal a big CC, do a GC, then signal another big CC, then do a CC.  In this event, we want to know that we should do a merging CC after the next GC, to cover the second signaled big CC.

One scenario I am somewhat concerned about that this code does not cover is non-merging-CC starvation: we could potentially end up doing merging CCs over and over, and not free up some memory.  It didn't seem to be a big problem in some simple browsing I was doing, but maybe it could be in general.  I'd probably solve this by adding yet another timer, that would force a non-merging CC at least once a minute or whatever.
Comment 12 Andrew McCreight [:mccr8] 2012-05-18 14:00:19 PDT
Created attachment 625251 [details] [diff] [review]
part 1: add JS hooks for scanning CC things in a compartment
Comment 13 Andrew McCreight [:mccr8] 2012-05-18 14:06:06 PDT
Created attachment 625259 [details] [diff] [review]
part 2: implement CompartmentParticipant, add hooks for it in CC
Comment 14 Andrew McCreight [:mccr8] 2012-05-18 14:08:03 PDT
Created attachment 625260 [details] [diff] [review]
part 3: schedule merging CC after killing windows
Comment 15 Andrew McCreight [:mccr8] 2012-05-18 14:32:38 PDT
Comment on attachment 625251 [details] [diff] [review]
part 1: add JS hooks for scanning CC things in a compartment

Bill, how reasonable/unreasonable does this look?  I forgot when I was talking to you in IRC before that I'd made my custom scanning function by modifying IterateCells, the function you recommended I use.  I can probably use IterateCells directly, with a callback wrapper, if that's cleaner.
Comment 16 Bill McCloskey (:billm) 2012-05-21 18:12:22 PDT
Comment on attachment 625251 [details] [diff] [review]
part 1: add JS hooks for scanning CC things in a compartment

This seems okay. What do you need the wrapper map callback for?
Comment 17 Andrew McCreight [:mccr8] 2012-05-21 18:37:10 PDT
(In reply to Bill McCloskey (:billm) from comment #16)
> This seems okay. What do you need the wrapper map callback for?
There are two kinds of edges out of stuff in compartments: pointers to GC things in other compartments, and C++ pointers.  For the former, I scan the list of cross-compartment wrappers, assuming that it contains pointers to other compartments (outgoing edges, in the tracing sense).  Is that right?  Now that I think about it, I may be wrong there.
Comment 18 Bill McCloskey (:billm) 2012-05-21 18:40:45 PDT
(In reply to Andrew McCreight [:mccr8] from comment #17)
> There are two kinds of edges out of stuff in compartments: pointers to GC
> things in other compartments, and C++ pointers.  For the former, I scan the
> list of cross-compartment wrappers, assuming that it contains pointers to
> other compartments (outgoing edges, in the tracing sense).  Is that right? 
> Now that I think about it, I may be wrong there.

That's most of them. Unfortunately there are more. My intention with bug 742841 is to gather all the cross-compartment references into a single map. It's stalled right now, but I could get back to it if you need it.
Comment 19 Andrew McCreight [:mccr8] 2012-05-21 19:04:42 PDT
Ah, that's unfortunate.  Are the debugger objects just the first ones you happened to start patching?  Or can I just disable this optimization when there are any debugger objects. ;)  I could also limit this to content compartments or something.
Comment 20 Andrew McCreight [:mccr8] 2012-05-21 19:21:05 PDT
Well, if it is just these debugger objects, then I can just disable it for the debugger compartment.  Though I'm not sure I want to add jsfriendapi hooks for telling if a compartment is a debugger compartment or not.
Comment 21 Bill McCloskey (:billm) 2012-05-22 10:57:14 PDT
The debugger is the only thing I know of that has cross-compartment pointers that aren't in the wrapper map. I'll try to get that patch ready soon. The alternatives don't sound very appealing.
Comment 22 Andrew McCreight [:mccr8] 2012-05-22 11:15:52 PDT
Awesome, thanks!
Comment 23 Andrew McCreight [:mccr8] 2012-05-22 14:24:40 PDT
My current heuristic for triggering these isn't perfect:

1. GC(T+473.5) Total Time: 169.2ms, Compartments Collected: 271, Total Compartments: 314  // SET_NEW_DOCUMENT
2. CC(T+478.4) duration: 70ms, suspected: 2303, visited: 32071 RCed and 95 merged GCed, collected: 1838 RCed and 10 GCed (1848 waiting for GC)
3. GC(T+482.5) Total Time: 42.7ms, Compartments Collected: 244, Total Compartments: 314  // CC_WAITING
4. GC(T+483.5) Total Time: 88.9ms, Compartments Collected: 287, Total Compartments: 287  // FULL_GC_TIMER
5. CC(T+484.8) duration: 395ms, suspected: 211, visited: 30187 RCed and 183877 GCed, collected: 29395 RCed and 183419 GCed (212814 waiting for GC)

What happens here is that we trigger a GC after closing 4 TC tabs, then do our merged CC, but for some reason the compartmental GC didn't unmark enough to actually collect much.  Then we have another few GCs, including a full GC, and then finally we get a CC that frees things, but we already did our merged GC and we get a big CC.

This doesn't seem to happen much, though, even in the presence of compartmental GCs.  One possible approach would be to keep a set of globals we've designated as being cleared, and then track on an individual basis whether they have been GCed, and somehow use that to drive our decision to merge or not.
Comment 24 Andrew McCreight [:mccr8] 2012-05-22 17:41:43 PDT
One possible cause for this problem is that I opened a bunch of TC tabs, then opened a blank tab, which I kept in the foreground.  Then I let the TC tabs sit for a while.  If this caused them to stop running JS (because they were in the background and I wasn't interacting with them), then the compartmental GC wouldn't visit them in step 1.  There seems to be some provision for that in nsJSContext::GC, but maybe with CPG that isn't sufficient?  I'll have to investigate further.
Comment 25 Olli Pettay [:smaug] 2012-05-22 23:31:16 PDT
Should we mark closed nsJSContexts active?
Comment 26 Andrew McCreight [:mccr8] 2012-05-23 09:53:25 PDT
(In reply to Olli Pettay [:smaug] from comment #25)
> Should we mark closed nsJSContexts active?
That sounds like a good idea!  That will avoid the problem in Comment 24 without any further changes, I think, and sounds like a good idea to boot.  I filed bug 757884 for that.
Comment 27 Andrew McCreight [:mccr8] 2012-05-31 10:52:08 PDT
I kind of want to use JSTracer instead of the new GCThingCallback (to make handling these children more uniform), but it uses indirect marking and passing in a reference to a key of crossCompartmentWrappers seems a little sketchy.
Comment 28 Andrew McCreight [:mccr8] 2012-06-15 18:21:15 PDT
new rebased and refactored stack of patches: https://tbpl.mozilla.org/?tree=Try&rev=0947d87e45c3
Comment 29 Andrew McCreight [:mccr8] 2012-06-16 11:01:12 PDT
Oddly this caused a lot of Bug 625273 failures.  I still need to make forced CC non-merging.  Maybe that will help.
Comment 30 Andrew McCreight [:mccr8] 2012-06-20 09:59:39 PDT
https://tbpl.mozilla.org/?tree=Try&rev=ce41e3516087
https://tbpl.mozilla.org/?tree=Try&rev=fe43e6e3e568

Two more try runs, two more rounds of the weird failure on OSX.  I get some other failures in Mochitest-chrome on Linux, including (two times) a crash from OOM.  That at least is more sensible!

I may work around this by only merging content compartments.  Those are probably most of what we care about anyways.  I just hope that that isn't merely papering over failures.
Comment 31 Andrew McCreight [:mccr8] 2012-06-20 15:15:00 PDT
In my local testing, the PAGE_HIDE GC doesn't always gray out the JS Object (Window) any more, when I close techcrunch, which is unfortunate.  We end up with page close -> PAGE_HIDE GC -> merged CC that doesn't traverse much JS -> CC_WAITING GC -> unmerged CC that traverses a ton of JS.  Maybe I should do something more robust to trigger merged CCs, like looking for gray JS Object (Window)s.
Comment 32 Bill McCloskey (:billm) 2012-06-20 15:18:15 PDT
(In reply to Andrew McCreight [:mccr8] from comment #31)
> In my local testing, the PAGE_HIDE GC doesn't always gray out the JS Object
> (Window) any more, when I close techcrunch, which is unfortunate.  We end up
> with page close -> PAGE_HIDE GC -> merged CC that doesn't traverse much JS
> -> CC_WAITING GC -> unmerged CC that traverses a ton of JS.  Maybe I should
> do something more robust to trigger merged CCs, like looking for gray JS
> Object (Window)s.

Doesn't the PAGE_HIDE GC happen 5 seconds after the page is closed? That seems like more than enough time to close things down. Do the intervening GC or CC somehow free up something that allows us to recognize that the window is gray?
Comment 33 Andrew McCreight [:mccr8] 2012-06-20 15:27:42 PDT
The PAGE_HIDE GC is the first GC or CC thing that runs after the page close.  I'm not sure why the window ends up black.  The GC heap dump from the start of the CCs wasn't very useful.
Comment 34 Andrew McCreight [:mccr8] 2012-06-20 16:20:11 PDT
It turns out it is easy enough to look for inactive windows with gray global objects, so I think I'll do that to trigger merging, rather than a weird heuristic about killing windows, and tracking whether we've GCed in between.
Comment 35 Andrew McCreight [:mccr8] 2012-06-23 17:40:42 PDT
I added some logging to see how long CCs were, and I think I was getting oranges because merged CC times were becoming horrifically long, a few seconds.  More disturbingly, they were intermixed with a few unmerged CCs that were very fast, which maybe could happen if you have huge compartments that are mostly black, but have a few gray objects in them.

Anyways, never merging system compartments seems to have fixed the oranges, but maybe I can do some further tweaking.
Comment 36 Andrew McCreight [:mccr8] 2012-06-25 13:30:43 PDT
Created attachment 636472 [details] [diff] [review]
part 1: add JS hooks for scanning CC things in a compartment
Comment 37 Andrew McCreight [:mccr8] 2012-06-26 08:46:44 PDT
Created attachment 636726 [details] [diff] [review]
part 2: add flag to control JS CC traversal behavior
Comment 38 Andrew McCreight [:mccr8] 2012-06-26 08:48:37 PDT
Created attachment 636727 [details] [diff] [review]
part 3: add shim to NoteJSChild to allow it to be reused
Comment 39 Andrew McCreight [:mccr8] 2012-06-26 09:45:22 PDT
Created attachment 636751 [details] [diff] [review]
part 4: define CompartmentParticipant
Comment 40 Andrew McCreight [:mccr8] 2012-06-26 09:51:06 PDT
Created attachment 636755 [details] [diff] [review]
part 5: add support for JSCompartment merging to the CC
Comment 41 Andrew McCreight [:mccr8] 2012-06-26 09:52:48 PDT
Created attachment 636756 [details] [diff] [review]
part 6: indicate in error console if we did a merging CC

DoMergingCC() is a stub that will be filled in later.
Comment 42 Andrew McCreight [:mccr8] 2012-06-26 09:56:25 PDT
Created attachment 636759 [details] [diff] [review]
part 7: indicate if CycleCollectNow is forced

If a cycle collection is forced, you probably care more about thoroughness than performance, so we shouldn't merge compartments. This patch lays the groundwork for that by adding an argument to CycleCollectNow that indicates if it is forced or not.
Comment 43 Andrew McCreight [:mccr8] 2012-06-26 10:14:11 PDT
Created attachment 636764 [details] [diff] [review]
part 8: do a merging CC when there's a gray global

This is the patch that actually turns on compartment merging CCs.  It schedules a merged CC when the parent of a JSContext's "global object" is gray, and that parent is in a non-system compartment.  This is the best way I could come up with to find the global object of the nsGlobalWindow.

The idea is that when a window object is marked gray, is when we're probably going to have a ton of content JS that needs to be cleaned up. This open-ended approach is nice, because it does not rely on trying to guess when that will happen. On the minus side, if a window is leaking, we could end up merging all the time. The next patch attempts to mitigate the problem there.
Comment 44 Andrew McCreight [:mccr8] 2012-06-26 11:34:59 PDT
Here's what the first 3 CCs after a page close of TechCrunch look like, without merging:

CC(T+30.5) duration: 13ms, suspected: 369, visited: 5000 RCed and 4831 GCed, collected: 4123 RCed and 1196 GCed (5319 waiting for GC)
ForgetSkippable 12 times before CC, min: 0 ms, max: 2 ms, avg: 0 ms, total: 9 ms, removed: 3779

CC(T+37.0) duration: 59ms, suspected: 350, visited: 5960 RCed and 29541 GCed, collected: 5375 RCed and 26634 GCed (32009 waiting for GC)
ForgetSkippable 3 times before CC, min: 0 ms, max: 2 ms, avg: 1 ms, total: 4 ms, removed: 295

CC(T+43.4) duration: 7ms, suspected: 63, visited: 1193 RCed and 3060 GCed, collected: 24 RCed and 0 GCed (24 waiting for GC)
ForgetSkippable 2 times before CC, min: 0 ms, max: 2 ms, avg: 1 ms, total: 3 ms, removed: 198

-----

Here's what the look like with merging:

CC(T+42.7) duration: 10ms, suspected: 229, visited: 2002 RCed and 3057 merged GCed, collected: 361 RCed and 9 GCed (370 waiting for GC)
ForgetSkippable 8 times before CC, min: 0 ms, max: 2 ms, avg: 0 ms, total: 7 ms, removed: 2613

CC(T+49.1) duration: 18ms, suspected: 331, visited: 6582 RCed and 3168 merged GCed, collected: 5412 RCed and 110 GCed (5522 waiting for GC)
ForgetSkippable 2 times before CC, min: 1 ms, max: 3 ms, avg: 2 ms, total: 4 ms, removed: 121

CC(T+55.1) duration: 7ms, suspected: 36, visited: 1147 RCed and 3056 GCed, collected: 3 RCed and 0 GCed (3 waiting for GC)
ForgetSkippable 2 times before CC, min: 0 ms, max: 2 ms, avg: 1 ms, total: 2 ms, removed: 132

-----

The first one isn't much faster, but the middle one, where the work is actually done, takes less than a third the time without merging (it has about 1/10th the JS nodes in the graph).  Then the third CC for both (after the merging is done because the page has been cleaned up) is the same in both.
Comment 45 Andrew McCreight [:mccr8] 2012-06-26 11:35:45 PDT
I should say that TechCrunch is an unusually JS-heavy webpage, so this is a best-case scenario.  I tried this before with the HTML5 spec and it made no difference.
Comment 46 Andrew McCreight [:mccr8] 2012-06-26 11:41:29 PDT
Comment on attachment 636726 [details] [diff] [review]
part 2: add flag to control JS CC traversal behavior

parts 2 and 3 shouldn't change behavior
Comment 47 Andrew McCreight [:mccr8] 2012-06-26 11:43:24 PDT
Comment on attachment 636751 [details] [diff] [review]
part 4: define CompartmentParticipant

This is the most technically complex patch, even though it is pretty short in terms of lines of code, so I'd appreciate it if the two of you could look at it.
Comment 48 Andrew McCreight [:mccr8] 2012-06-26 11:48:22 PDT
Comment on attachment 636755 [details] [diff] [review]
part 5: add support for JSCompartment merging to the CC

Most of the changes here (but not all!) are just threading around aMergeCompartment.
Comment 49 Olli Pettay [:smaug] 2012-06-26 11:49:14 PDT
Comment on attachment 636726 [details] [diff] [review]
part 2: add flag to control JS CC traversal behavior


>+enum TraverseSelect {
>+    TRAVERSE_CPP,
>+    TRAVERSE_FULL
>+};
Nit, I don't remember xpconnect's coding style rules, but elsewhere { should be on its own line.
Comment 50 Andrew McCreight [:mccr8] 2012-06-26 11:49:58 PDT
Comment on attachment 636756 [details] [diff] [review]
part 6: indicate in error console if we did a merging CC

Sorry for all the bug spam...
Comment 51 Andrew McCreight [:mccr8] 2012-06-26 11:51:50 PDT
Comment on attachment 636764 [details] [diff] [review]
part 8: do a merging CC when there's a gray global

This patch is a bit hacky.  Let me know if you have any better ideas how to trigger a merged CC...
Comment 52 Andrew McCreight [:mccr8] 2012-06-26 11:55:49 PDT
Created attachment 636815 [details] [diff] [review]
part 9: don't merge too much

I almost forgot this patch.  The purpose of this patch is to avoid extreme behavior on the merging scheduling.  We shouldn't merge more than 3 times in a row, and when we don't merge, we should un-merge at least 3 times in a row, to make sure we actually break up garbage.

The main bad scenario I am thinking of here is when we leak a document, we could end up with having a gray document all of the time.  We don't want to merge all the time, because then we may leak.  With this patch, we'll get 3 merged CCs in a row, then 3 unmerged.  It may also help with automated tests that open and close pages rapidly.
Comment 53 Olli Pettay [:smaug] 2012-06-26 12:02:54 PDT
Comment on attachment 636764 [details] [diff] [review]
part 8: do a merging CC when there's a gray global


>+AnyGrayGlobalParent()
>+{
>+  if (!nsJSRuntime::sRuntime) {
>+    return false;
>+  }
>+  JSContext *iter = nsnull;
>+  JSContext *cx;
>+  while ((cx = JS_ContextIterator(nsJSRuntime::sRuntime, &iter))) {
>+    if (JSObject *global = JS_GetGlobalObject(cx)) {
>+      if (JSObject *parent = js::GetObjectParent(global)) {
>+        if (js::GCThingIsMarkedGray(parent) &&
>+            !js::IsSystemCompartment(js::GCThingCompartment(parent))) {
>+          return true;
>+        }
>+      }
>+    }
>+  }
>+  return false;
>+}
Comment 54 Olli Pettay [:smaug] 2012-06-26 12:06:35 PDT
Comment on attachment 636815 [details] [diff] [review]
part 9: don't merge too much


> DoMergingCC(bool aForced)
> {
>-  return !aForced && AnyGrayGlobalParent();
>+  // Don't merge too many times in a row, and do at least a minimum
>+  // number of unmerged CCs in a row.
>+  static const int minConsecutiveUnmerged = 3;
>+  static const int maxConsecutiveMerged = 3;
>+
>+  static int unmergedNeeded = 0;
>+  static int mergedInARow = 0;
PRInt32?
And static variables should be sVariableName
Comment 55 Bill McCloskey (:billm) 2012-06-26 12:13:45 PDT
Comment on attachment 636472 [details] [diff] [review]
part 1: add JS hooks for scanning CC things in a compartment

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

::: js/src/jsfriendapi.cpp
@@ +465,5 @@
>      return reinterpret_cast<gc::Cell *>(thing)->isMarked(gc::GRAY);
>  }
>  
> +JS_FRIEND_API(JSCompartment*)
> +js::GCThingCompartment(void *thing)

I think we've been using GetX in the APIs (e.g., GetContextCompartment). So this should probably be GetGCThingCompartment.

@@ +482,5 @@
> +    }
> +}
> +
> +JS_FRIEND_API(void)
> +js::VisitGrayObjectCells(JSCompartment *compartment, GCThingCallback *cellCallback, void *data)

No need for a wrapper around the jsgc.cpp version. It's fine to have a JS_FRIEND_API function in jsgc.cpp.

::: js/src/jsgc.cpp
@@ +4090,5 @@
>  
>  namespace gc {
>  
> +void
> +VisitGrayObjectCells(JSCompartment *compartment, GCThingCallback *cellCallback, void *data)

Could you call this IterateGrayCells instead, and move it out of the gc namespace so it's like IterateCells?
Comment 56 Olli Pettay [:smaug] 2012-06-26 12:40:25 PDT
Comment on attachment 636755 [details] [diff] [review]
part 5: add support for JSCompartment merging to the CC


>+    JSCompartment *MergeCompartment(void *gcthing) {
>+        if (!mMergeCompartments)
>+            return nsnull;
>+        JSCompartment *comp = js::GCThingCompartment(gcthing);
>+        if (js::IsSystemCompartment(comp)) {
>+            return nsnull;
>+        }
>+        return comp;
>+    }
Please be consistent with {} usage.
Comment 57 Andrew McCreight [:mccr8] 2012-06-26 13:06:21 PDT
(In reply to Bill McCloskey (:billm) from comment #55)
> Could you call this IterateGrayCells instead, and move it out of the gc
> namespace so it's like IterateCells?
Maybe IterateGrayObjects?  It only iterates over objects.  Or at least, that is the intent. ;)  Because objects are the only GCthings that can contain pointers to C++ objects.
Comment 58 Bill McCloskey (:billm) 2012-06-26 13:09:21 PDT
Comment on attachment 636751 [details] [diff] [review]
part 4: define CompartmentParticipant

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

This all seems surprisingly reasonable.

I realize now that VisitGrayObjectCells only visits objects. So maybe a better name would be IterateGrayObjects.

::: js/xpconnect/src/nsXPConnect.cpp
@@ +2667,5 @@
> + * compartment, where one is garbage and the other is live. If we merge the entire
> + * compartment, the cycle collector will think that both are alive.
> + *
> + * We don't have to worry about losing track of a garbage cycle, because any such garbage
> + * cycle incorrectly identified as live must pass contain at least one C++ to JS edge,

"pass contain" -> "contain"
Comment 59 Andrew McCreight [:mccr8] 2012-06-26 13:14:12 PDT
(In reply to Bill McCloskey (:billm) from comment #58)
> This all seems surprisingly reasonable.
Yeah, deciding how to schedule merging CCs ended up being the most annoying part.  I think the core merging code worked the very first time I tried it.
Comment 60 Andrew McCreight [:mccr8] 2012-06-26 13:48:47 PDT
Previously I'd only been testing closing a tab, but I checked just now and merging works fine when navigating too.  I navigated a bit, then I saw some --DOMWINDOW in the log, then we got a merging CC.  There then were some more --DOMWINDOW for various follow button things, then a non-merging CC, but the non-merging CC was pretty fast and didn't have that many JS objects, so it looks like the first merging one caught everything important.
Comment 61 Andrew McCreight [:mccr8] 2012-06-26 14:29:48 PDT
Created attachment 636877 [details] [diff] [review]
(folded patch for testing)
Comment 64 :Ms2ger (⌚ UTC+1/+2) 2012-06-29 04:43:56 PDT
Comment on attachment 636815 [details] [diff] [review]
part 9: don't merge too much

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

::: dom/base/nsJSEnvironment.cpp
@@ +2987,5 @@
> +  static int unmergedNeeded = 0;
> +  static int mergedInARow = 0;
> +
> +  MOZ_ASSERT(0 <= unmergedNeeded <= minConsecutiveUnmerged);
> +  MOZ_ASSERT(0 <= mergedInARow <= maxConsecutiveMerged);

You know that doesn't work, right?
Comment 65 Andrew McCreight [:mccr8] 2012-06-29 06:31:32 PDT
(In reply to :Ms2ger from comment #64)
> You know that doesn't work, right?
Oops, I guess I need to && stuff together?  I've spent too much time using theorem provers...
Comment 66 Andrew McCreight [:mccr8] 2012-06-29 09:05:58 PDT
Created attachment 637926 [details] [diff] [review]
part 10: followup to fix assertion
Comment 67 :Ms2ger (⌚ UTC+1/+2) 2012-06-29 09:07:15 PDT
Comment on attachment 637926 [details] [diff] [review]
part 10: followup to fix assertion

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

rs=me
Comment 68 Andrew McCreight [:mccr8] 2012-06-29 09:10:54 PDT
Thanks for catching this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf1e126bccad
Comment 69 Ryan VanderMeulen [:RyanVM] 2012-06-30 12:45:05 PDT
https://hg.mozilla.org/mozilla-central/rev/cf1e126bccad
Comment 70 Olli Pettay [:smaug] 2012-07-05 02:52:17 PDT
Looks like there is a small drop in P75 and P95 and VISITED_GCED is lower too.
So, the patch seems to actually work :)

Note You need to log in before you can comment on or make changes to this bug.