Closed
Bug 1399160
Opened 7 years ago
Closed 7 years ago
Make CCGraphBuilder::BuildGraph to check the budget more often
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: smaug, Assigned: smaug)
Details
Attachments
(2 files)
3.75 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
3.79 KB,
patch
|
Details | Diff | Splinter Review |
Maybe this is a bit better way than just decrease kNumNodesBetweenTimeChecks
Attachment #8907129 -
Flags: review?(continuation)
Comment 1•7 years ago
|
||
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•7 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•7 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.
Assignee | ||
Comment 4•7 years ago
|
||
Comment 5•7 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? 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.
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
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/644d9742cf75
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Assignee: nobody → bugs
You need to log in
before you can comment on or make changes to this bug.
Description
•