Closed Bug 1247554 Opened 4 years ago Closed 4 years ago

Budget creation of AGRs per pixel area

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox45 --- wontfix
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: jnicol, Assigned: jnicol)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

In bug 1231818 I originally intended to budget the creation of AGRs due to animated offsets or margins based on area, but instead opted to actually try to enforce layers.max-active in FrameLayerBuilder.

While enforcing the max number of layers was still worthwhile, budgeting AGRs by area is also necessary, as the other patch is still vulnerable to fewer but larger layers.

While discussing the other ticket, we decided it was best not to repurpose the will-change budget into a general AGR budget, as it is something specifically requested by web pages.
See Also: → 1231818
The public interface remains the same, but internally to
nsDisplayListBuilder make the will-change budget an instance of a more
general "AGR budget". This will allow us to easily budget the creation
of AGRs due to other reasons as well.

Review commit: https://reviewboard.mozilla.org/r/35149/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35149/
Attachment #8719869 - Flags: review?(bgirard)
https://reviewboard.mozilla.org/r/35151/#review31739

::: layout/base/nsDisplayList.cpp:599
(Diff revision 1)
> +#ifdef MOZ_WIDGET_ANDROID

Consider MOZ_GFX_OPTIMIZE_MOBILE if this is really what you want
Comment on attachment 8719869 [details]
MozReview Request: Bug 1247554 - Refactor will-change budget internals into general-purpose AGR budget; r?benwa

https://reviewboard.mozilla.org/r/35149/#review31951

As we discussed on IRC I don't think these changes make sense. Will-change doesn't budget on AGR, it does a budget on layer surface area (pixels). I don't think we should make these changes at all.
Attachment #8719869 - Flags: review?(bgirard)
Attachment #8719870 - Flags: review?(bgirard)
Comment on attachment 8719870 [details]
MozReview Request: Bug 1247554 - Budget creation of AGRs due to animated offsets and margins; r?benwa

https://reviewboard.mozilla.org/r/35151/#review31953

These changes seem ok, maybe. But it's not really my area. I'd suggest to have someone from layout review it. Perhaps one of: :tn, :roc, :mattwoodrow
Attachment #8719869 - Attachment is obsolete: true
Attachment #8719870 - Attachment is obsolete: true
Previous attempt to unify will-change and this into an "AGR budget" did not make sense, because will-change does not use AGRs. It would perhaps have made sense as a "Layerization budget" instead.

But this time I'm not trying to unify them - we'll have a will-change budget and a separate "AGR budget". Which are similar, yes, but are used differently and behave slightly differently.

Currently only restricts AGRs due to animated offset/margins. could be extended to more.

Unlike will-change opted for only 1 budget rather than 1 per PresContext. Wasn't sure about that, but thought we wouldn't want a page with multiple PresContexts to be allowed to use too much memory?

Limits chosen are fairly arbitrary - didn't want to make too much difference to desktop but want to limit mobile a fair bit.
In nsDisplayListBuilder keep track of the total area of frames which
have been made animated geometry roots due to having animated offset or
margin properties. This is in order to reduce the total area of created
layers. Once the area has reached a certain limit do not allow any more
frames to be AGRs for this reason.

Currently this only applies to AGRs created due to frames having
animated offset or margin properties. This is because we believe this is
a major source of over-layerisation and too high memory usage. This
could easily be extended to include the creation of AGRs due to other
reasons too.

Note that this technically limits the area of AGR frames, not the area
of layers, though it will have a fairly direct impact.

Review commit: https://reviewboard.mozilla.org/r/35491/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35491/
Attachment #8720884 - Flags: review?(matt.woodrow)
Attachment #8720883 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8720883 [details]
MozReview Request: Bug 1247554 - Mark nsPresContext::GetVisibleArea as const; r=mattwoodrow

https://reviewboard.mozilla.org/r/35489/#review32173
Comment on attachment 8720884 [details]
MozReview Request: Bug 1247554 - Budget creation of AGRs by frame area; r=mattwoodrow

https://reviewboard.mozilla.org/r/35491/#review32177

::: layout/base/nsDisplayList.h:1230
(Diff revision 1)
> +  uint32_t mAGRBudget;

mUsedAGRBudget? Or mAllocatedAGRBudget.

::: layout/base/nsDisplayList.cpp:1362
(Diff revision 1)
> +    onBudget = mAGRBudget + cost <

Just use this branch all the time, as discussed on irc.
Attachment #8720884 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8720884 [details]
MozReview Request: Bug 1247554 - Budget creation of AGRs by frame area; r=mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35491/diff/1-2/
Latest version makes changes recommended in comment 11.

Try run - https://treeherder.mozilla.org/#/jobs?repo=try&revision=8303ddb055eb&selectedJob=16918834
hits some intermittents but nothing caused by this

requesting checkin please
Keywords: checkin-needed
Comment on attachment 8720884 [details]
MozReview Request: Bug 1247554 - Budget creation of AGRs by frame area; r=mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35491/diff/2-3/
Oops, somehow lost the initialization of mUsedAGRBudget in the previous pushes.
try run rebased on central with mUsedAGRBudget initialization: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eb88e7da740
https://hg.mozilla.org/mozilla-central/rev/09d06de1aba4
https://hg.mozilla.org/mozilla-central/rev/c59e8b0fd0ad
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8720884 [details]
MozReview Request: Bug 1247554 - Budget creation of AGRs by frame area; r=mattwoodrow

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Excessive memory usage, possibly crashes
[Describe test coverage new/current, TreeHerder]: Nightly, automated tests
[Risks and why]: Low to medium, but I believe the worst case is that some things that would normally be layerized will not be, resulting in lower performance. On Android this is exactly what we want, because we're creating too many layers and OOMing.
[String/UUID change made/needed]: None
Attachment #8720884 - Flags: approval-mozilla-beta?
Attachment #8720884 - Flags: approval-mozilla-aurora?
Not recent regressions, should help with our currently bad OOM issues on android, marking affected for current branches.
Comment on attachment 8720884 [details]
MozReview Request: Bug 1247554 - Budget creation of AGRs by frame area; r=mattwoodrow

This has had a couple of days on Nightly, and should help us decrease OOM crashes on android.  Taking it for aurora though it may be a bit risky
Attachment #8720884 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8720883 [details]
MozReview Request: Bug 1247554 - Mark nsPresContext::GetVisibleArea as const; r=mattwoodrow

[Triage Comment]
Attachment #8720883 - Flags: approval-mozilla-aurora+
Comment on attachment 8720884 [details]
MozReview Request: Bug 1247554 - Budget creation of AGRs by frame area; r=mattwoodrow

As discussed on IRC, too late for 45.
Attachment #8720884 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.