751561 - Call forgetSkippable twice before CC

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Bug 747675 changed how we handle forgetSkippable.
Now tools, like about:cc, report more stuff in the CC because not all the cleaning up
is happening.
Patch coming.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=a204a6b1a41a

Comment 2

[Andrew McCreight (out until 2026) [:mccr8]]

Comment on attachment 620694 [details] [diff] [review]
patch

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

::: dom/base/nsJSEnvironment.cpp
@@ +185,5 @@
>  static PRUint32 sRemovedPurples = 0;
>  static PRUint32 sForgetSkippableBeforeCC = 0;
>  static PRUint32 sPreviousSuspectedCount = 0;
>  
> +static PRUint32 sCleanupsSinceLastGC = PR_UINT32_MAX;

Why do you init this to MAX instead of 0 like below?  I guess the init function is always called so it shouldn't really matter?

@@ +2961,3 @@
>    for (PRInt32 i = 0; i < aExtraForgetSkippableCalls; ++i) {
>      nsCycleCollector_forgetSkippable();
> +    ++sCleanupsSinceLastGC;

Maybe put this loop before the other loop?  Right now, if you ask for 2 extra forget skippables, but you haven't done any cleanups, you'll end up doing 4 before.  But maybe it doesn't matter...

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrew McCreight [:mccr8] from comment #2)  
> > +static PRUint32 sCleanupsSinceLastGC = PR_UINT32_MAX;
> 
> Why do you init this to MAX instead of 0 like below? 
Where below? It is set to 0 when GC has run


> >    for (PRInt32 i = 0; i < aExtraForgetSkippableCalls; ++i) {
> >      nsCycleCollector_forgetSkippable();
> > +    ++sCleanupsSinceLastGC;
> 
> Maybe put this loop before the other loop?  Right now, if you ask for 2
> extra forget skippables, but you haven't done any cleanups, you'll end up
> doing 4 before.
I want to have those 4 forgetSkippables. 
aExtraForgetSkippableCalls are for testing, and they are really extras

Comment 4

[Andrew McCreight (out until 2026) [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #3)
> > Why do you init this to MAX instead of 0 like below? 
> Where below? It is set to 0 when GC has run

Oh, I misread.  Shouldn't you init the value in the init method in nsJSEnvironment?  To MAX I guess.

> I want to have those 4 forgetSkippables. 
> aExtraForgetSkippableCalls are for testing, and they are really extras

Sounds good to me.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrew McCreight [:mccr8] from comment #4) 
> Oh, I misread.  Shouldn't you init the value in the init method in
> nsJSEnvironment?
Not sure why. And I'm not changing the initialization place here.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/82fad02b755a

Comment 7

[Justin Lebar (not reading bugmail)]

There was a big decrease in the number of ghost windows seen from the 5/3/12 to the 5/4/12 nightly (807403a04 -- 2db9df428).  Looking at the affected csets, this is the only patch which I can imagine affecting that measure.

If this is indeed responsible for the change, it's an awesome patch.

Comment 8

[Andrew McCreight (out until 2026) [:mccr8]]

That would be surprising, as this patch should only affect manually-triggered CCs.  But maybe it fixes a bug we didn't know we had. ;)