Make CCGraphBuilder::BuildGraph to check the budget more often

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
Maybe this is a bit better way than just decrease kNumNodesBetweenTimeChecks
Attachment #8907129 - Flags: review?(continuation)
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.
Attachment #8907129 - Flags: review?(continuation) → review+
(Assignee)

Comment 2

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

Comment 3

2 years ago
(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.
(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

2 years ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/644d9742cf75
Make CCGraphBuilder::BuildGraph to check the budget more often, r=mccr8
https://hg.mozilla.org/mozilla-central/rev/644d9742cf75
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee: nobody → bugs
You need to log in before you can comment on or make changes to this bug.