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.

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.

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.

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

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.

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.

I think the leaks I saw were somehow related to a_chrome_compartment<->another_chrome_compartment edges.

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

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.

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.

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 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 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?

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

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

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.

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.

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.

Awesome, thanks!

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.

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.

Should we mark closed nsJSContexts active?

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

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.

new rebased and refactored stack of patches: https://tbpl.mozilla.org/?tree=Try&rev=0947d87e45c3

Oddly this caused a lot of Bug 625273 failures. I still need to make forced CC non-merging. Maybe that will help.

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.

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.

(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?

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.

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.

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.

DoMergingCC() is a stub that will be filled in later.

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.

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.

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.

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

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

(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 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"

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

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.

Thanks for the quick reviews everybody! https://hg.mozilla.org/integration/mozilla-inbound/rev/ac3322d1a9dc https://hg.mozilla.org/integration/mozilla-inbound/rev/cadd1096dec0 https://hg.mozilla.org/integration/mozilla-inbound/rev/09e9fd1c5eea https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d9d9473b01 https://hg.mozilla.org/integration/mozilla-inbound/rev/06fe901437d5 https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f1af5a3094 https://hg.mozilla.org/integration/mozilla-inbound/rev/d101692ae80e https://hg.mozilla.org/integration/mozilla-inbound/rev/fe20c11fbdde https://hg.mozilla.org/integration/mozilla-inbound/rev/afaa7483d846

https://hg.mozilla.org/mozilla-central/rev/ac3322d1a9dc https://hg.mozilla.org/mozilla-central/rev/cadd1096dec0 https://hg.mozilla.org/mozilla-central/rev/09e9fd1c5eea https://hg.mozilla.org/mozilla-central/rev/f6d9d9473b01 https://hg.mozilla.org/mozilla-central/rev/06fe901437d5 https://hg.mozilla.org/mozilla-central/rev/d0f1af5a3094 https://hg.mozilla.org/mozilla-central/rev/d101692ae80e https://hg.mozilla.org/mozilla-central/rev/fe20c11fbdde https://hg.mozilla.org/mozilla-central/rev/afaa7483d846

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?

(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 on attachment 637926 [details] [diff] [review] part 10: followup to fix assertion Review of attachment 637926 [details] [diff] [review]: ----------------------------------------------------------------- rs=me

Thanks for catching this. https://hg.mozilla.org/integration/mozilla-inbound/rev/cf1e126bccad

Looks like there is a small drop in P75 and P95 and VISITED_GCED is lower too. So, the patch seems to actually work :)