Always use at minimum 4 slices for iCC?

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

36 Branch
mozilla36
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

4 years ago
(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 :)
This logic is supposed to do something along those lines:
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsCycleCollector.cpp#3637
(Assignee)

Comment 3

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

Comment 4

4 years ago
Created attachment 8529023 [details] [diff] [review]
icc_4_slices.diff

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

Comment 5

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

Comment 7

4 years ago
Created attachment 8529315 [details] [diff] [review]
only 4+ slices when we're painting something

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+
(Assignee)

Comment 9

4 years ago
(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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.