1399160 - Make CCGraphBuilder::BuildGraph to check the budget more often

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

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

Attachment: CCGraphBuilder_BuildGraph_budget_check.diff

Maybe this is a bit better way than just decrease kNumNodesBetweenTimeChecks

Comment 1

[Andrew McCreight [:mccr8]]

Comment on attachment 8907129 [details] [diff] [review]
CCGraphBuilder_BuildGraph_budget_check.diff

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

r=me with the change below

::: xpcom/base/nsCycleCollector.cpp
@@ +2319,5 @@
>      if (mCurrNode->AtBlockEnd()) {
>        SetLastChild();
>      }
>  
> +    aBudget.step(mNoteChildCount);

This should be kStep * (mNoteChildCount + 1), and then delete the next line. You have to scale it by kStep.

@@ +2411,5 @@
>    if (!aChild || !(aChild = CanonicalizeXPCOMParticipant(aChild))) {
>      return;
>    }
>  
> +  ++mNoteChildCount;

You might want to consider putting these first in the callbacks. It depends on how much of the overhead is just examining the object and doing the callback, compared to running the interesting part of it. Though we may filter out JS null values before we even get here so in that case it wouldn't matter.

Comment 2

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

(In reply to Andrew McCreight [:mccr8] from comment #1)
> Comment on attachment 8907129 [details] [diff] [review]
> CCGraphBuilder_BuildGraph_budget_check.diff
> 
> Review of attachment 8907129 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the change below
> 
> ::: xpcom/base/nsCycleCollector.cpp
> @@ +2319,5 @@
> >      if (mCurrNode->AtBlockEnd()) {
> >        SetLastChild();
> >      }
> >  
> > +    aBudget.step(mNoteChildCount);
> kip the 
> This should be kStep * (mNoteChildCount + 1), and then delete the next line.
> You have to scale it by kStep.
Why?


> > +  ++mNoteChildCount;
> 
> You might want to consider putting these first in the callbacks. It depends
> on how much of the overhead is just examining the object and doing the
> callback, compared to running the interesting part of it. Though we may
> filter out JS null values before we even get here so in that case it
> wouldn't matter.
Well, I explicitly decided to skip the null checks and such.

Comment 3

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

(In reply to Olli Pettay [:smaug] from comment #2)
> > This should be kStep * (mNoteChildCount + 1), and then delete the next line.
> > You have to scale it by kStep.
> Why?

But I can do that, since in practice it doesn't change anything.

Comment 4

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

Attachment: CCGraphBuilder_BuildGraph_budget_check_2.diff

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #2)
> > This should be kStep * (mNoteChildCount + 1), and then delete the next line.
> > You have to scale it by kStep.
> Why?

The way SliceBudget works is that it actually checks the counter every CounterStep operations, not every kNumNodesBetweenTimeChecks operations. If we want to do checks more often, by adjusting kNumNodesBetweenTimeChecks, then we need to scale it by kStep. Of course as you point out right now they are equal so it won't matter.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/644d9742cf75
Make CCGraphBuilder::BuildGraph to check the budget more often, r=mccr8

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/644d9742cf75