1201327 - Use DLBI for invalidating background-position changes

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

At the moment, changes to background-position invalidate the entire frame (+ descendants). That's wasteful.

Comment 1

[Markus Stange [:mstange]]

Attachment: wip

This patch makes us use SchedulePaint instead of RepaintFrame, but apparently DLBI doesn't pick up the background image geometry change.

Comment 2

[Markus Stange [:mstange]]

That's because DLBI only checks the background positioning area, which is the area that background-position is *relative to* (i.e. the frame's border rect). This isn't affacted by background-position.
DLBI needs to check the dest rect as well.

Comment 3

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1201327 - Rename mDestRect to mImageLayerDestRect. r?mattwoodrow

Bug 1201327 - Rename mDestRect to mImageLayerDestRect. r?mattwoodrow

Comment 4

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1201327 - Let DLBI detect background-position changes. r?mattwoodrow

Bug 1201327 - Let DLBI detect background-position changes. r?mattwoodrow

Comment 5

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1201327 - Don't repaint the whole frame subtree when background-position changes. r?dbaron

Bug 1201327 - Don't repaint the whole frame subtree when background-position changes. r?dbaron

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8679481 [details]
MozReview Request: Bug 1201327 - Rename mDestRect to mImageLayerDestRect. r?mattwoodrow

https://reviewboard.mozilla.org/r/23427/#review20943

Comment 7

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8679482 [details]
MozReview Request: Bug 1201327 - Let DLBI detect background-position changes. r?mattwoodrow

https://reviewboard.mozilla.org/r/23429/#review20947

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8679481 [details]
MozReview Request: Bug 1201327 - Rename mDestRect to mImageLayerDestRect. r?mattwoodrow

Bug 1201327 - Rename mDestRect to mImageLayerDestRect. r?mattwoodrow

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8679482 [details]
MozReview Request: Bug 1201327 - Let DLBI detect background-position changes. r?mattwoodrow

Bug 1201327 - Let DLBI detect background-position changes. r?mattwoodrow

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8679483 [details]
MozReview Request: Bug 1201327 - Don't repaint the whole frame subtree when background-position changes. r?dbaron

Bug 1201327 - Don't repaint the whole frame subtree when background-position changes. r?dbaron

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8679483 [details]
MozReview Request: Bug 1201327 - Don't repaint the whole frame subtree when background-position changes. r?dbaron

https://reviewboard.mozilla.org/r/23431/#review21335

::: layout/reftests/invalidation/background-position-1.html:4
(Diff revision 2)
> +<title>Changes to background-position should not cause things to repaint that don't intersect the background image.</title>

This title is misleading about what the test is testing... unless it's somehow testing lack of repainting in a way that I totally don't see.

(Is there a way you could actually test lack-of-repaint?)

::: layout/style/nsStyleStruct.cpp:2263
(Diff revision 2)
> -        hasVisualDifference = true;
> +        NS_UpdateHint(hint, NS_CombineHint(nsChangeHint_UpdateEffects, nsChangeHint_RepaintFrame));
> +        return hint;

This is a bit inconsistent.  In using NS_UpdateHint(hint, ...) you're acting as though what's currently in hint (which might also include SchedulePaint) matters, yet you're doing an early return and thus assuming that any hints produced by later layers don't matter.

(I assume that RepaintFrame completely subsumes the work that SchedulePaint does.  If that's not true, there's an actual bug here.)

Given that the only case that really matters to optimize for is no-change, I tend to think it's not worth micro-optimizing CalcDifference methods with early returns, and I think you should just drop the early return.

Also, I'm happy to see code change to use the pattern:
  hint |= nsChangeHint\_... | nsChangeHint\_...
although I've been too lazy (so far) to do a mass-change to get rid of NS_UpdateHint, NS_CombineHint, etc.

I also wonder if there are a bunch of other cases (for non-inherited properties) where we could replace RepaintFrame hints with SchedulePaint hints.

Comment 12

[Markus Stange [:mstange]]

(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #11)
> Comment on attachment 8679483 [details]
> MozReview Request: Bug 1201327 - Don't repaint the whole frame subtree when
> background-position changes. r?dbaron
> 
> https://reviewboard.mozilla.org/r/23431/#review21335
> 
> ::: layout/reftests/invalidation/background-position-1.html:4
> (Diff revision 2)
> > +<title>Changes to background-position should not cause things to repaint that don't intersect the background image.</title>
> 
> This title is misleading about what the test is testing... unless it's
> somehow testing lack of repainting in a way that I totally don't see.
> 
> (Is there a way you could actually test lack-of-repaint?)

Yes, the lack-of-repaint is tested using class="reftest-no-paint".
http://hg.mozilla.org/mozilla-central/file/451a18579143/layout/tools/reftest/README.txt#l454

> 
> ::: layout/style/nsStyleStruct.cpp:2263
> (Diff revision 2)
> > -        hasVisualDifference = true;
> > +        NS_UpdateHint(hint, NS_CombineHint(nsChangeHint_UpdateEffects, nsChangeHint_RepaintFrame));
> > +        return hint;
> 
> This is a bit inconsistent.  In using NS_UpdateHint(hint, ...) you're acting
> as though what's currently in hint (which might also include SchedulePaint)
> matters, yet you're doing an early return and thus assuming that any hints
> produced by later layers don't matter.
> 
> (I assume that RepaintFrame completely subsumes the work that SchedulePaint
> does.  If that's not true, there's an actual bug here.)
> 
> Given that the only case that really matters to optimize for is no-change, I
> tend to think it's not worth micro-optimizing CalcDifference methods with
> early returns, and I think you should just drop the early return.

Yeah, I wasn't happy about this. I'll drop the early return.

> Also, I'm happy to see code change to use the pattern:
>   hint |= nsChangeHint\_... | nsChangeHint\_...
> although I've been too lazy (so far) to do a mass-change to get rid of
> NS_UpdateHint, NS_CombineHint, etc.

Oh, good. I'll change it in the lines that I'm touching.

> I also wonder if there are a bunch of other cases (for non-inherited
> properties) where we could replace RepaintFrame hints with SchedulePaint
> hints.

I bet there are lots, but we need to go through them one by one and make sure that DLBI covers all the cases.

Comment 13

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fec3431b0b127da40a84181bff9d039ad23050ee
Bug 1201327 - Rename mDestRect to mImageLayerDestRect. r=mattwoodrow

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0a00d6915bb7c858ae7e1d94c7507b76e23eddd
Bug 1201327 - Let DLBI detect background-position changes. r=mattwoodrow

https://hg.mozilla.org/integration/mozilla-inbound/rev/21cabf6ab3e9a254cdc9370986f300213b346cb2
Bug 1201327 - Don't repaint the whole frame subtree when background-position changes. r=dbaron

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fec3431b0b12
https://hg.mozilla.org/mozilla-central/rev/b0a00d6915bb
https://hg.mozilla.org/mozilla-central/rev/21cabf6ab3e9

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/fec3431b0b12
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b0a00d6915bb
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/21cabf6ab3e9

Comment 16

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 17

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

Does this (and its dependencies) affect 46 and 47? It looks like the backouts from bug 1244258 fixed this for 45. But we haven't resolved these issues in 46 yet.

Comment 18

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

Markus if you could clarify what needs backout here that will be super helpful!
Wes, something needs backout according to my conversation with Markus.

Comment 19

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

Adding a ni? for tomcat since he'll likely get to this sooner than I will.

Comment 20

[Markus Stange [:mstange]]

All three patches that landed in this bug need to be backed out on 46. There is a backout patch in bug 1244258 which applied on 45; I'm not sure if it also applies on 46.

Comment 21

[Carsten Book [:Tomcat]]

(In reply to Markus Stange [:mstange] from comment #20)
> All three patches that landed in this bug need to be backed out on 46. There
> is a backout patch in bug 1244258 which applied on 45; I'm not sure if it
> also applies on 46.

yep applied cleanly and landed in    https://hg.mozilla.org/releases/mozilla-aurora/rev/8248eb3a6e69

Comment 22

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

Marking this affected for 46 (since it was fixed in 45, then the changes were backed out of 45 and 46) . I'm still not sure whether we need to track this bug for 46 or whether the issues are being resolved in other bugs. Markus, sorry I am not following this 100%, can you help me straighten it out next week?

Comment 23

[Markus Stange [:mstange]]

Actually, since it's also been backed out on 45, let's mark it affected for 45 as well.
This bug does not need to be tracked; it's just a performance improvement and not a particularly crucial one.

Did this answer you question?

Comment 24

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

Sure, thanks.

Comment 25

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

Markus should this get backed out of 46 as well considering the regressions? Thanks.

Comment 26

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

The backout in https://bugzilla.mozilla.org/show_bug.cgi?id=1244258 that is.

Comment 27

[Markus Stange [:mstange]]

Yes, I'll back it out on Aurora again. Should I just land the backout with a=lizzard?

Comment 28

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

That's fine for 47 aurora if you can associate it with bug 1244258 (or wherever you are doing the backout) 
46 is heading into beta 9 later this week. Are both 47 and 46 affected?

Comment 29

[Markus Stange [:mstange]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #28)
> That's fine for 47 aurora if you can associate it with bug 1244258 (or
> wherever you are doing the backout)

Ok, sure, I'll do it in bug 1244258.

> 46 is heading into beta 9 later this week. Are both 47 and 46 affected?

The backout already landed on 46 in comment 21. We don't need to do anything for 46 here.

Comment 30

[Markus Stange [:mstange]]

I've backed it out on Aurora 47.