Call forgetSkippable before CC

RESOLVED FIXED in mozilla12

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

12 Branch
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
Attachment #591958 - Flags: review?(continuation)
OS: Linux → All
Hardware: x86_64 → All
(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.
(Assignee)

Comment 2

5 years ago
(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.
(Assignee)

Updated

5 years ago
Depends on: 716014
(Assignee)

Updated

5 years ago
Summary: Call forgetSkippable before CC and call compartment gc more often → Call forgetSkippable before CC
(Assignee)

Comment 3

5 years ago
Created attachment 592103 [details] [diff] [review]
patch, without compartment GC

I'll put the compartment gc thing to bug 716014
Attachment #591958 - Attachment is obsolete: true
Attachment #591958 - Flags: review?(continuation)
Attachment #592103 - Flags: review?(continuation)
Thanks!  Sorry I didn't finish many reviews yesterday.
(Assignee)

Comment 5

5 years ago
(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 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.
Attachment #592103 - Flags: review?(continuation) → review-
(Assignee)

Comment 7

5 years ago
Note to myself, this can't land before Bug 721548 or there will be leaks.
Depends on: 721548
(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?
(Assignee)

Comment 9

5 years ago
(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
No longer depends on: 721548
(Assignee)

Comment 10

5 years ago
(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.
(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.
(Assignee)

Comment 12

5 years ago
Created attachment 592141 [details] [diff] [review]
patch
Attachment #592103 - Attachment is obsolete: true
Attachment #592141 - Flags: review?(continuation)
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.
Attachment #592141 - Flags: review?(continuation) → review+
(Assignee)

Comment 14

5 years ago
Created attachment 592376 [details] [diff] [review]
+null check to fix xpcshell test
https://hg.mozilla.org/mozilla-central/rev/de4f382f487b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Blocks: 723081
Depends on: 723064
You need to log in before you can comment on or make changes to this bug.