Open Bug 1284143 Opened 8 years ago Updated 2 years ago

cycle collection should not become really slow as a result of leaked (and no longer active) windows

Categories

(Core :: Cycle Collector, defect, P3)

Product:
Core
Component:
Cycle Collector
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox50 --- affected

People

(Reporter: dbaron, Unassigned)

Details

(Keywords: perf)

One of the ways that a bunch of Mozilla developers have been noticing significant memory leaks (e.g., bug 1266856) in the past year or so is that cycle collection pauses become excruciatingly slow. While it's nice that we catch more leaks, it's a problem that leaks lead to such horrible performance problems. It seems to me that this should be avoidable. In most cases the leaked objects are in a window / compartment that has no activity at all and is unconnected to any other compartments that are active. We should be able to use this in some way to avoid adding (in XPCJSRuntime) the JS objects in compartments that have not had any script executed (or any other activity that could cause changes to their object graph) to the cycle collection graph every single cycle collection. (I was discussing this with Kyle last week. We were discussing needing the knowledge of which compartments are reachable from other compartments -- which we could build from cross-compartment wrappers. And I think that would be necessary to avoid taking multiple cycles of cycle collection to clean up graphs that weren't cleaned up immediately after window destruction, but I'm not sure that that's critical, and I can't right now think of why that would be necessary to ensure that we clean things up, as long as destruction of any wrappers going into a compartment does mark that compartment as active.) Or, to put it another way: I haven't worked out the details, but it seems like we ought to be able to avoid traversing all of the JS objects in compartments in closed windows that haven't changed since the last cycle collection. Though it's probably worth checking before we start any large projects that we wouldn't traverse into them via things in the purple buffer anyway.
(In reply to David Baron :dbaron: ⌚️UTC-4 (review requests must explain patch) from comment #0) > It seems to me that this should be avoidable. In most cases the leaked > objects are in a window / compartment that has no activity at all and is > unconnected to any other compartments that are active. I'd guess the most common case leaked objects are in windows/compartments which _are_connected_ to an active windows (Google Reader case). That is why we explicitly optimize such window (etc) objects out from the CC graph. But yes, we should be able not traverse unconnected windows, at least not all the time. There are existing bugs for that. > We should be able to > use this in some way to avoid adding (in XPCJSRuntime) the JS objects in > compartments that have not had any script executed (or any other activity > that could cause changes to their object graph) to the cycle collection > graph every single cycle collection. Yup. Need to do full CC still occasionally, since activity in other compartments may have changed whether those objects have become garbage. I think one option is also to use zone-merging for such possibly leaked compartments to reduce time spent in CC. Another thing we may want to look at is doing better in incremental CC, so that even if the graph becomes huge, we just end up incrementalizing the collection well enough so that max pause times stay low enough.
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #1) > (In reply to David Baron :dbaron: ⌚️UTC-4 (review requests must explain patch) from comment #0) > > We should be able to > > use this in some way to avoid adding (in XPCJSRuntime) the JS objects in > > compartments that have not had any script executed (or any other activity > > that could cause changes to their object graph) to the cycle collection > > graph every single cycle collection. > Yup. Need to do full CC still occasionally, since activity in other > compartments may have changed whether those objects have become garbage. > I think one option is also to use zone-merging for such possibly leaked > compartments to reduce time spent in CC. I'm not convinced that we do. The reason we need to add JS roots at the C++ -> JS boundary to the set of cycle collection roots (in addition to the roots from the purple buffer) is that we don't have reference-count traffic for the JS object graph, so we don't know when an edge was removed that could cause something to be part of a garbage cycle that was not part of a garbage cycle before. However, if there's a completely contained part of the JS object graph (a compartment) that has had no changes since the last cycle collection, I don't see why we need to add the roots that point into that compartment, as long as there are no changes to the object graph in it. I don't see how a new garbage cycle could form in that case without roots elsewhere that we're already adding to the cycle collector. That said, I don't know whether we can reliably detect all changes to the object graph.
I've been thinking about this problem for a while, but I haven't come up with any great ideas. Bug 1179894 is about one possible approach to this problem. (In reply to David Baron :dbaron: ⌚️UTC-4 (review requests must explain patch) from comment #0) > It seems to me that this should be avoidable. In most cases the leaked > objects are in a window / compartment that has no activity at all and is > unconnected to any other compartments that are active. IIRC at least at some point we tracked which compartments had script executed in them since the last GC, which isn't quite the right epoch for CC, but it is at least close. Tracking which compartments have been entered might be sufficient as a "dirty bit" for compartments. > (I was discussing this with Kyle last week. We were discussing needing the > knowledge of which compartments are reachable from other compartments -- > which we could build from cross-compartment wrappers. CCWs are not sufficient: you need to know about all JS -> C++ references, for the JS in the compartment, and which other compartments that C++ in turn may hold. A simpler mechanism for dealing with this is zone merging, which I added in bug 754495 (though it is not actually triggering at the moment). If you decide to merge a zone, then you trace through all of its objects at once, which is much faster. (It reduced CC time something like 66% when collecting JS heavy pages.) Maybe merging zones is not as good as compartments, for this case. One thing to keep in mind is that leaking pages have tons of DOM nodes, and skipping compartments won't help with that. We could also do something like noticing that a window was in the CC graph over and over again, and then just pretend it is definitely alive most of the time. That would reduce user bother a fair amount. (This is basically bug 1179894). (In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #1) > Another thing we may want to look at is doing better in incremental CC, so > that even if the graph becomes huge, we just end up incrementalizing the > collection well enough so that max pause times stay low enough. Slice times are very good right now, for graph building (which is where all the time goes for these leaked cases). The problem is that with leaked windows CC can take a second or more of actual work, and we have capped the number of slices of work we do, so that we don't fall behind, so we end up just synchronously finishing the CC in a single chunk. Perhaps bumping up the slice time to 100ms or even more when CCs are taking a long time would avoid this. (And then dropping it down again if CCs start taking a single slice?) (In reply to David Baron :dbaron: ⌚️UTC-4 (review requests must explain patch) from comment #2) > The reason we need to add JS roots at the C++ -> JS boundary to the set of > cycle collection roots (in addition to the roots from the purple buffer) is > that we don't have reference-count traffic for the JS object graph, so we > don't know when an edge was removed that could cause something to be part of > a garbage cycle that was not part of a garbage cycle before. There is an additional reason we must add all gray JS objects to the CC graph: we cannot tell if a given gray JS object is being held alive from C++ without tracing through all C++ holding onto JS. I think your idea from comment 0 avoids this problem because we would be checking that no other compartment is reached from the current compartment, and treating all things in that compartment as alive.
(In reply to Andrew McCreight [:mccr8] from comment #3) > There is an additional reason we must add all gray JS objects to the CC > graph: we cannot tell if a given gray JS object is being held alive from C++ > without tracing through all C++ holding onto JS. I think your idea from > comment 0 avoids this problem because we would be checking that no other > compartment is reached from the current compartment, and treating all things > in that compartment as alive. I think I don't understand what comment 0 is proposing or how it would work. A C++ object a' can have its wrapper living in compartment A, yet hold other C++ objects 'b' which have wrappers living in compartment B. If the 'a' becomes suspected , 'b' or the whole B can be now garbage, yet nothing has been executed in B or otherwise touched B.
(In reply to Olli Pettay [:smaug] (on vacation for couple of days) from comment #4) > (In reply to Andrew McCreight [:mccr8] from comment #3) > > There is an additional reason we must add all gray JS objects to the CC > > graph: we cannot tell if a given gray JS object is being held alive from C++ > > without tracing through all C++ holding onto JS. I think your idea from > > comment 0 avoids this problem because we would be checking that no other > > compartment is reached from the current compartment, and treating all things > > in that compartment as alive. > > > I think I don't understand what comment 0 is proposing or how it would work. > A C++ object a' can have its wrapper living in compartment A, yet hold other > C++ objects 'b' which have > wrappers living in compartment B. If the 'a' becomes suspected , 'b' or the > whole B can be now garbage, yet nothing has been executed in B or otherwise > touched B. What I'm suggesting is that we change the algorithm that we use to approximate the purple buffer logic for JS (because we don't have reference count traffic for JS objects). If a C++ object becomes suspected because of its reference counting, we would still put it into the purple buffer and traverse from it as a cycle collection root as usual. But I'm suggesting that we not add the JS roots associated with a compartment into the cycle collection graph if there's been no activity in that compartment since the last cycle collection. (Whether cross-compartment wrappers would then need some special handling requires some thought, but I think at least for some definitions of "activity in a compartment" they probably don't.) This is based on the principle that the reason we need to add JS roots to cycle collection is to simulate the logic in the Bacon & Ranjan paper where a decrease in reference count turns an object purple: since JS doesn't have reference counts, we just add all the edges from C++ -> JS to the graph as roots (but then stop tracing when we hit things reachable from a displayed window), since that will cover the cases that (a) the JS GC doesn't cover and (b) where we've changed the JS graph in a way that creates a new garbage cycle that was not a garbage cycle at the previous cycle collection (i.e., a cycle that was unreachable from the C++ side at the last cycle collection but still live due to edges from JS). My hope -- although it's not clear whether this is actually the case -- is that with that more limited set of cycle collection roots, we wouldn't end up traversing into the JS graphs of ghost windows. I could certainly be wrong, and if I'm wrong, there's no point. I think in your example, there wouldn't be a problem because we'd still trace the graph to find the garbage cycle, since we'll have traversed to the wrapper 'a' and we'll then traverse from there through to all of the necessary objects in compartment B. The cycle collection algorithm only requires that the garbage cycle be reachable from a cycle collection root in a single way -- which should be related the last thing that made the cycle non-garbage before it became garbage.
(In reply to David Baron :dbaron: ⌚️UTC-4 (review requests must explain patch) from comment #5) > But I'm suggesting > that we not add the JS roots associated with a compartment into the cycle > collection graph if there's been no activity in that compartment since the > last cycle collection. cycle collection or garbage collection? If we can make GC unmarkgray the whole compartment, CC wouldn't do anything with it. So, should we add a flag to compartment telling whether there has been activity between GCs, and if not, unmark all objects? CC would then automatically optimize out everything. > since JS doesn't have > reference counts, we just add all the edges from C++ -> JS to the graph as > roots (but then stop tracing when we hit things reachable from a displayed > window), Well, we need to trace only gray marked JS. Hmm, so this all would work quite well also in case we had compartmental GC working well - IIRC it marks effectively other compartment black while GCing certain compartments.
(In reply to David Baron :dbaron: ⌚️UTC-4 (review requests must explain patch) from comment #5) > I could certainly be wrong, and if I'm wrong, there's no point. The way to implement this would probably be to just filter out a particular set of compartments from the CC graph. Then you don't have to worry about missing particular referents. You still have to deal with CCWs somehow, which is why I think using compartment merging instead of complete filtering would be better. Or something like Olli said, where we could make the logic in TraceBlackJS to add the globals of additional things we want to make the CC not visit (doing it there instead of with UnmarkGray is more efficient).
(In reply to Olli Pettay [:smaug] (on vacation for couple of days) from comment #6) > (In reply to David Baron :dbaron: ⌚️UTC-4 (review requests must explain > patch) from comment #5) > > But I'm suggesting > > that we not add the JS roots associated with a compartment into the cycle > > collection graph if there's been no activity in that compartment since the > > last cycle collection. > cycle collection or garbage collection? I meant cycle collection. > If we can make GC unmarkgray the whole compartment, CC wouldn't do anything > with it. > So, should we add a flag to compartment telling whether there has been > activity between GCs, and if not, unmark all objects? CC would then > automatically optimize out everything. But that's not what I want to happen. We might still need to trace through stuff in the compartment to find real cycles. What I want to do is avoid getting into it at all if there's no chance of new cycles since the last CC, which I hope should be possible for things that are truly ghost windows that have been leaked. > > since JS doesn't have > > reference counts, we just add all the edges from C++ -> JS to the graph as > > roots (but then stop tracing when we hit things reachable from a displayed > > window), > Well, we need to trace only gray marked JS. I think that's the same as what I said inside the (). If not, then I'm not understanding. (In reply to Andrew McCreight [:mccr8] from comment #7) > The way to implement this would probably be to just filter out a particular > set of compartments from the CC graph. Then you don't have to worry about > missing particular referents. You still have to deal with CCWs somehow, > which is why I think using compartment merging instead of complete filtering > would be better. Or something like Olli said, where we could make the logic > in TraceBlackJS to add the globals of additional things we want to make the > CC not visit (doing it there instead of with UnmarkGray is more efficient). I *think* you're talking about something quite different from what I was talking about. It's not clear to me how to safely filter entire compartments out of the graph. Though perhaps it's worthwhile to do for some but not all cycle collections, once we've noticed that a window was leaked, just to avoid the huge pauses every time when it seems unlikely that we'd be able to free memory. But there still could be garbage cycles in that window that become collectable later, and filtering the compartment out of the graph every time would prevent them from being collected. (This seems more likely with things like iframes that have been removed from a parent document than with true ghost windows in toplevel tabs.)
Ok, so you want something that can be done all the time without causing any leaks. So you mean something like, don't add a JS object via C++ unless we have directly suspected the C++ object (or we can transitively reach the C++ object from suspected object), and we somehow can tell that we haven't touched anything in the compartment of the JS object. I see. We'd have to add all C++ roots to a particular compartment if we add any, per the final paragraph of comment 3.
I believe the idea is don't add a JS object via C++ unless the C++ object is transitively selected *or* we *have* touched something in that compartment. I think we also have to add all CCWs *into* compartments we have touched. Consider C++ || JS || Thing 1 C++ ----------------++--------------------> Thing 1 JS | || | | || | | |+------------------------+--------------- Compartment boundary | || | | || | Thing 2 C++ <---------------++--------------------- Thing 2 JS <---- Definitely black root If the edge from definitely black root to Thing 2 JS goes away we won't know to start traversing from Thing 2 JS, and we won't traverse the Thing 1 C++ to JS edge because nothing changed in the compartment of Thing 1. But if we add the CCW from Thing 1 JS to Thing 2 JS as a "purple" object then we'll find the cycle.
Yeah, JS things which were touched/owned by CCW, must be added to suspected JS thing list. Both sides of CCW. Marking the relevant compartments 'accessed' is hopefully fast enough. CCW is super hot code (bug 1237058 etc.) and currently way too slow. How would GC deal with possibly optimized out compartments? C++ || JS || Definitely black root --> Thing 1 C++ ----------------++--------------------> Thing 1 JS | || | | || | | || | | || | | || | Thing 2 C++ <---------------++--------------------- Thing 2 JS if we have just a single compartment and C++ Thing 1 is certainly alive and skipped by canSkip. canSkip marks also C++ -> Thing 1 JS edge black, which propagates to other JS edges. Now, if right after this the state of Thing 1 C++ changes so that it isn't definitely live anymore, CC still sees everything in JS black and keeps the compartment alive. How and when do we kill the cycle? If GC later marks everything gray, we still try to optimize out the compartment since nothing has happened in the compartment. I guess any black->gray marking during GC should also be 'accessed the compartment'. Only if all the JS objects were gray before and after previous GC, then we'd try to optimize it out.
Is this more or less important now that we don't have addons doing lots of wild things with our internals?
Priority: -- → P3
(In reply to Nathan Froyd [:froydnj] from comment #12) > Is this more or less important now that we don't have addons doing lots of > wild things with our internals? This is still as much of a problem. I have some other bugs filed on this issue, but I'm not sure what the right way forward is.
Severity: normal → S3
Component: XPCOM → Cycle Collector
You need to log in before you can comment on or make changes to this bug.