1247554 - Budget creation of AGRs per pixel area

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jamie Nicol [:jnicol] out of office until 6th Jan]

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.

Comment 1

[Jamie Nicol [:jnicol] out of office until 6th Jan]

Attachment: MozReview Request: Bug 1247554 - Refactor will-change budget internals into general-purpose AGR budget; r?benwa

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/

Comment 2

[Jamie Nicol [:jnicol] out of office until 6th Jan]

Attachment: MozReview Request: Bug 1247554 - Budget creation of AGRs due to animated offsets and margins; r?benwa

Review commit: https://reviewboard.mozilla.org/r/35151/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35151/

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

I am excited.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

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 5

[Benoit Girard (:BenWa)]

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.

Comment 6

[Benoit Girard (:BenWa)]

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

Comment 7

[Jamie Nicol [:jnicol] out of office until 6th Jan]

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.

Comment 8

[Jamie Nicol [:jnicol] out of office until 6th Jan]

Attachment: MozReview Request: Bug 1247554 - Mark nsPresContext::GetVisibleArea as const; r=mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/35489/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35489/

Comment 9

[Jamie Nicol [:jnicol] out of office until 6th Jan]

Attachment: MozReview Request: Bug 1247554 - Budget creation of AGRs by frame area; r=mattwoodrow

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/

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8720883 [details]
MozReview Request: Bug 1247554 - Mark nsPresContext::GetVisibleArea as const; r=mattwoodrow

https://reviewboard.mozilla.org/r/35489/#review32173

Comment 11

[Matt Woodrow (:mattwoodrow)]

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.

Comment 12

[Jamie Nicol [:jnicol] out of office until 6th Jan]

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/

Comment 13

[Jamie Nicol [:jnicol] out of office until 6th Jan]

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

Comment 14

[Jamie Nicol [:jnicol] out of office until 6th Jan]

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/

Comment 15

[Jamie Nicol [:jnicol] out of office until 6th Jan]

Oops, somehow lost the initialization of mUsedAGRBudget in the previous pushes.

Comment 16

[Jamie Nicol [:jnicol] out of office until 6th Jan]

try run rebased on central with mUsedAGRBudget initialization: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eb88e7da740

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09d06de1aba4
https://hg.mozilla.org/integration/mozilla-inbound/rev/c59e8b0fd0ad

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/09d06de1aba4
https://hg.mozilla.org/mozilla-central/rev/c59e8b0fd0ad

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Not recent regressions, should help with our currently bad OOM issues on android, marking affected for current branches.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8720883 [details]
MozReview Request: Bug 1247554 - Mark nsPresContext::GetVisibleArea as const; r=mattwoodrow

[Triage Comment]

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/80ce31ccfe48
https://hg.mozilla.org/releases/mozilla-aurora/rev/cb06a8b792a7

Comment 24

[Sylvestre Ledru [:Sylvestre]]

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.