Last Comment Bug 721543 - Call forgetSkippable before CC
: Call forgetSkippable before CC
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: mozilla12
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on: 716014 723064
Blocks: 705582 723081
  Show dependency treegraph
 
Reported: 2012-01-26 14:53 PST by Olli Pettay [:smaug]
Modified: 2012-02-02 05:54 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (22.61 KB, patch)
2012-01-26 14:53 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
patch, without compartment GC (15.63 KB, patch)
2012-01-27 05:26 PST, Olli Pettay [:smaug]
continuation: review-
Details | Diff | Splinter Review
patch (15.75 KB, patch)
2012-01-27 08:18 PST, Olli Pettay [:smaug]
continuation: review+
Details | Diff | Splinter Review
+null check to fix xpcshell test (15.79 KB, patch)
2012-01-28 01:41 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2012-01-26 14:53:59 PST
Created attachment 591958 [details] [diff] [review]
patch

The patch changes also nsIDOMWindowUtils so that garbageCollect and cycleCollect
methods take some more (optional) parameters.

I'd really like to try the compartment GC. It makes also most of the CCs significantly
faster.
Comment 1 Andrew McCreight [:mccr8] 2012-01-26 16:18:44 PST
(In reply to Olli Pettay [:smaug] from comment #0)
> I'd really like to try the compartment GC. It makes also most of the CCs
> significantly faster.

How hard would it be to split out the compartmental GC stuff?  Bill should be the one who takes a look at that, and if it is separate he could more easily test it and see what its effect on the GC is.  Compartmental GCs are done only rarely right now, so it may have some weird effect on things.

The reason that doing more compartmental GCs probably make the CCs much faster is that doing a compartmental GC clears the bits in all of the other compartments, so they will all be non-gray, which will allow your CC optimizations to ignore a lot of it.  In essence, we're getting compartmental CCs: only C++ held by one compartment (or by none at all) are being traversed.  That's good, but we have to be careful to schedule a CC whenever there is a global GC, and before the next compartmental GC, to catch any cycles that are reachable from multiple compartments.
Comment 2 Olli Pettay [:smaug] 2012-01-27 01:40:57 PST
(In reply to Andrew McCreight [:mccr8] from comment #1)
> How hard would it be to split out the compartmental GC stuff?
Not too hard.

 
> The reason that doing more compartmental GCs probably make the CCs much
> faster is that doing a compartmental GC clears the bits in all of the other
> compartments, so they will all be non-gray, which will allow your CC
> optimizations to ignore a lot of it.  In essence, we're getting
> compartmental CCs:
Exactly! that is a very good thing.
Comment 3 Olli Pettay [:smaug] 2012-01-27 05:26:38 PST
Created attachment 592103 [details] [diff] [review]
patch, without compartment GC

I'll put the compartment gc thing to bug 716014
Comment 4 Andrew McCreight [:mccr8] 2012-01-27 05:30:59 PST
Thanks!  Sorry I didn't finish many reviews yesterday.
Comment 5 Olli Pettay [:smaug] 2012-01-27 06:10:09 PST
(In reply to Andrew McCreight [:mccr8] from comment #1)
> That's good, but we have to be careful to schedule a CC
> whenever there is a global GC, and before the next compartmental GC, to
> catch any cycles that are reachable from multiple compartments.
DOMGCFinishedCallback already handles that.
Comment 6 Andrew McCreight [:mccr8] 2012-01-27 06:44:20 PST
Comment on attachment 592103 [details] [diff] [review]
patch, without compartment GC

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

Looks good.  This is a pretty cool little mechanism.

r- because I'd like to see the "garbage held while idle" problem addressed.  It sounds kind of silly, but I'm concerned about future MemShrink bug where somebody has a giant heap that mysteriously shrinks when they press the CC button.  Though really, loading about:memory is probably enough to jar the CC into action. ;)

::: dom/base/nsJSEnvironment.cpp
@@ +3316,5 @@
>        sFirstCollectionTime = now;
>      }
>  
> +    NS_NAMED_MULTILINE_LITERAL_STRING(kFmt,
> +      NS_LL("CC(T+%.1f) collected: %lu (%lu waiting for GC), suspected: %lu, duration: %llu ms.\n")

Could you maybe move duration to before collected?  I think that's the thing people usually care about.  Also, maybe remove the waiting for GC stat.  It seems like an odd heuristic, and it can be computed by looking at the error console if somebody cares.

@@ +3401,5 @@
> +  } else {
> +    sPreviousSuspectedCount = 0;
> +    nsJSContext::KillCCTimer();
> +    if (nsCycleCollector_suspectedCount() > 500) {
> +      nsJSContext::CycleCollectNow();

This isn't aggressive enough.  There was a problem with the GC in Fx4 (that was fixed in Fx7) where it wasn't running when you left it overnight, which caused fragmentation and the browser running out of memory.  I guess if the suspected count is growing slowly or not at all, you probably aren't going to blow up memory, but we still want to make sure any garbage gets freed in at least a somewhat timely fashion, say, 5 or 10 minutes.  There are two problems I can think of that should be addressed.  

First, memory could be held alive by suspected nodes*, so we want to make sure that if there are any suspected nodes, we should run the CC every 5-10 minutes.  Second, a GC could create a garbage cycle on the JS side, so you want a similar mechanism to ensure that a CC is run at least once in the 5-10 minutes after a GC.

These mechanisms will ensure that we free things every so often, but will still not trigger if the browser is truly idle.  For the first one, once we run the suspected count down to 0, if the browser isn't doing anything, it will stop running.  For the second one, it will only trigger when the GC triggers, so we're not really idle.

*In the Bacon CC paper, they found that in one benchmark the CC only freed a handful of cycles, but because those nodes held alive giant arrays, the benchmark would OOM without the CC.

@@ +3501,5 @@
> +    sCCTimerFireCount = 0;
> +    CallCreateInstance("@mozilla.org/timer;1", &sCCTimer);
> +    sCCTimer->InitWithFuncCallback(CCTimerFired, nsnull,
> +                                   NS_CC_SKIPPABLE_DELAY,
> +                                   nsITimer::TYPE_REPEATING_SLACK);

What is a repeating slack timer?  It keeps firing until you tell it to stop, and it isn't too picky about how precise its firing is?

::: dom/base/nsJSEnvironment.h
@@ +198,5 @@
>    static void KillCCTimer();
>  
>    virtual void GC(js::gcreason::Reason aReason);
>  
> +  static bool PreviousCollectionWasGC();

This isn't a very good name, as it tracks ForgetSkippable and not just CCs.  Maybe LastGCCleanedup or CleanupSinceLastGC? (for this function and the variable in the cpp file)  LastGCForgotten? ;)  Okay that last one would be confusing.
Comment 7 Olli Pettay [:smaug] 2012-01-27 07:15:42 PST
Note to myself, this can't land before Bug 721548 or there will be leaks.
Comment 8 Andrew McCreight [:mccr8] 2012-01-27 07:26:10 PST
(In reply to Olli Pettay [:smaug] from comment #7)
> Note to myself, this can't land before Bug 721548 or there will be leaks.

That's surprising.  Why is that?
Comment 9 Olli Pettay [:smaug] 2012-01-27 07:36:27 PST
(In reply to Andrew McCreight [:mccr8] from comment #6)
> ::: dom/base/nsJSEnvironment.cpp
> @@ +3316,5 @@
> >        sFirstCollectionTime = now;
> >      }
> >  
> > +    NS_NAMED_MULTILINE_LITERAL_STRING(kFmt,
> > +      NS_LL("CC(T+%.1f) collected: %lu (%lu waiting for GC), suspected: %lu, duration: %llu ms.\n")
> 
> Could you maybe move duration to before collected? 
Well, I'm  not changing that. Better to do changes in a different bug.


> I think that's the thing
> people usually care about.  Also, maybe remove the waiting for GC stat.
I actually look at gc stat all the time


> @@ +3401,5 @@
> > +  } else {
> > +    sPreviousSuspectedCount = 0;
> > +    nsJSContext::KillCCTimer();
> > +    if (nsCycleCollector_suspectedCount() > 500) {
> > +      nsJSContext::CycleCollectNow();
> 
> This isn't aggressive enough.  There was a problem with the GC in Fx4 (that
> was fixed in Fx7) where it wasn't running when you left it overnight, which
> caused fragmentation and the browser running out of memory.
Bug #?

 
> @@ +3501,5 @@
> > +    sCCTimerFireCount = 0;
> > +    CallCreateInstance("@mozilla.org/timer;1", &sCCTimer);
> > +    sCCTimer->InitWithFuncCallback(CCTimerFired, nsnull,
> > +                                   NS_CC_SKIPPABLE_DELAY,
> > +                                   nsITimer::TYPE_REPEATING_SLACK);
> 
> What is a repeating slack timer?  It keeps firing until you tell it to stop,
> and it isn't too picky about how precise its firing is?
Yeah.


 
> This isn't a very good name, as it tracks ForgetSkippable and not just CCs. 
> Maybe LastGCCleanedup or CleanupSinceLastGC?
I'll use CleanupSinceLastGC
Comment 10 Olli Pettay [:smaug] 2012-01-27 07:37:19 PST
(In reply to Andrew McCreight [:mccr8] from comment #8)
> (In reply to Olli Pettay [:smaug] from comment #7)
> > Note to myself, this can't land before Bug 721548 or there will be leaks.
> 
> That's surprising.  Why is that?
Because documents would have wrong CCGeneration.
Comment 11 Andrew McCreight [:mccr8] 2012-01-27 07:43:29 PST
(In reply to Olli Pettay [:smaug] from comment #9)
> > Could you maybe move duration to before collected? 
> Well, I'm  not changing that. Better to do changes in a different bug.
okay, sure.

> > I think that's the thing
> > people usually care about.  Also, maybe remove the waiting for GC stat.
> I actually look at gc stat all the time

Ok, we should leave it in then.  In the future we may want to think about tuning this.  In a very recent Nightly, you can now see why the GC is running, and for me, I'm seeing a lot of GCs due to the CC.  Ideally, we'd track the amount of JS stuff that is freed.

> > @@ +3401,5 @@
> > > +  } else {
> > > +    sPreviousSuspectedCount = 0;
> > > +    nsJSContext::KillCCTimer();
> > > +    if (nsCycleCollector_suspectedCount() > 500) {
> > > +      nsJSContext::CycleCollectNow();
> > 
> > This isn't aggressive enough.  There was a problem with the GC in Fx4 (that
> > was fixed in Fx7) where it wasn't running when you left it overnight, which
> > caused fragmentation and the browser running out of memory.
> Bug #?
I'll poke around to find this.
Comment 12 Olli Pettay [:smaug] 2012-01-27 08:18:37 PST
Created attachment 592141 [details] [diff] [review]
patch
Comment 13 Andrew McCreight [:mccr8] 2012-01-27 10:23:04 PST
Comment on attachment 592141 [details] [diff] [review]
patch

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

So you just always run the CC every 5 minutes?  Sounds reasonable to me.
Comment 14 Olli Pettay [:smaug] 2012-01-28 01:41:33 PST
Created attachment 592376 [details] [diff] [review]
+null check to fix xpcshell test
Comment 15 Andrew McCreight [:mccr8] 2012-01-30 16:26:08 PST
https://hg.mozilla.org/mozilla-central/rev/de4f382f487b

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