Last Comment Bug 751561 - Call forgetSkippable twice before CC
: Call forgetSkippable twice before CC
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on:
Blocks: 747675
  Show dependency treegraph
 
Reported: 2012-05-03 07:15 PDT by Olli Pettay [:smaug]
Modified: 2012-05-20 14:25 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.87 KB, patch)
2012-05-03 07:34 PDT, Olli Pettay [:smaug]
continuation: review+
Details | Diff | Review

Description Olli Pettay [:smaug] 2012-05-03 07:15:08 PDT
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 2 Andrew McCreight [:mccr8] 2012-05-03 08:10:24 PDT
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] 2012-05-03 09:08:25 PDT
(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 [:mccr8] 2012-05-03 09:12:33 PDT
(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] 2012-05-03 09:16:17 PDT
(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] 2012-05-03 09:28:52 PDT
https://hg.mozilla.org/mozilla-central/rev/82fad02b755a
Comment 7 Justin Lebar (not reading bugmail) 2012-05-20 14:22:26 PDT
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 [:mccr8] 2012-05-20 14:25:43 PDT
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. ;)

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