Closed Bug 1105089 Opened 9 years ago Closed 9 years ago

Always use at minimum 4 slices for iCC?

Categories

(Core :: XPCOM, defect)

36 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: smaug, Assigned: smaug)

Details

Attachments

(1 file, 1 obsolete file)

Currently http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsCycleCollector.cpp?rev=91e6e73aa5e6#3627 relies on budget to be over, but
could we reduce the risk for over budget by actually always run
IdlePhase, GraphBuildingPhase+, ScanAndCollectWhitePhase, CleanupPhase

Currently it is possible that GraphBuildingPhase takes almost 5ms, and then we enter to
unlinking which runs sync and takes quite some time.



(but getting late now or so, since I don't now see what guarantees sane 
slices in http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp?rev=683c4915bcee#1692)
(In reply to Olli Pettay [:smaug] from comment #0)
> (but getting late now or so, since I don't now see what guarantees sane 
> slices in
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.
> cpp?rev=683c4915bcee#1692)
Yes, just too late here :)
continueSlice = aBudget.isUnlimited() || mResults.mNumSlices < 3; is the part I was wondering.
Why mResults.mNumSlices < 3?

And also forcing GraphBuildingPhase to be always in different slice than ScanAndCollectWhitePhase.
Attached patch icc_4_slices.diff (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=3290ff1ebd45

I was thinking something like this. During games and such, we should try to have
as short slices as possible so that we don't take too much time of the 16ms
slice.
Maybe we want to have this behavior only if we're painting all the time, and otherwise
keep the old behavior or something. (though, 4 slices isn't that bad)
(In reply to Olli Pettay [:smaug] from comment #5)
> Maybe we want to have this behavior only if we're painting all the time, and
> otherwise keep the old behavior or something. (though, 4 slices isn't that bad)

Yeah, I think that makes sense.  With 4 slices we end up taking a lot longer, so it isn't very power efficient, but that's a minor issue in the grand scheme of things.
https://tbpl.mozilla.org/?tree=Try&rev=72ea97a67693

In FlightOfTheNavigator we get usually 4 slices which are very short, but
normal browsing can then have longer but fewer pauses.
Assignee: nobody → bugs
Attachment #8529023 - Attachment is obsolete: true
Attachment #8529315 - Flags: review?(continuation)
Comment on attachment 8529315 [details] [diff] [review]
only 4+ slices when we're painting something

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

Thanks!

::: xpcom/base/nsCycleCollector.cpp
@@ +3601,5 @@
>  bool
>  nsCycleCollector::Collect(ccType aCCType,
>                            SliceBudget& aBudget,
> +                          nsICycleCollectorListener* aManualListener,
> +                          bool aPreferShorterSlices)

Maybe put this argument after the budget argument, as it is kind of related to the budget?

@@ +3642,5 @@
>          // (There's no need to check if we've finished graph building, because
>          // if we haven't, we've already exceeded our budget, and will finish
>          // this slice anyways.)
> +        continueSlice = aBudget.isUnlimited() ||
> +          (mResults.mNumSlices < 3 && !aPreferShorterSlices);

This is a little awkward, but ok.
Attachment #8529315 - Flags: review?(continuation) → review+
(In reply to Andrew McCreight [:mccr8] from comment #8)
> 
> Maybe put this argument after the budget argument, as it is kind of related
> to the budget?
Well, I wanted to have the default value for aPreferShorterSlices ( = false).
https://hg.mozilla.org/mozilla-central/rev/7f94a959f345
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.