Closed Bug 1201327 Opened 4 years ago Closed 4 years ago

Use DLBI for invalidating background-position changes

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox43 --- affected
firefox45 --- affected
firefox46 --- affected
firefox47 --- affected
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

At the moment, changes to background-position invalidate the entire frame (+ descendants). That's wasteful.
Attached patch wip (obsolete) — Splinter Review
This patch makes us use SchedulePaint instead of RepaintFrame, but apparently DLBI doesn't pick up the background image geometry change.
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.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Bug 1201327 - Rename mDestRect to mImageLayerDestRect. r?mattwoodrow
Attachment #8679481 - Flags: review?(matt.woodrow)
Bug 1201327 - Let DLBI detect background-position changes. r?mattwoodrow
Attachment #8679482 - Flags: review?(matt.woodrow)
Bug 1201327 - Don't repaint the whole frame subtree when background-position changes. r?dbaron
Attachment #8679483 - Flags: review?(dbaron)
Attachment #8666801 - Attachment is obsolete: true
Comment on attachment 8679481 [details]
MozReview Request: Bug 1201327 - Rename mDestRect to mImageLayerDestRect. r?mattwoodrow

https://reviewboard.mozilla.org/r/23427/#review20943
Attachment #8679481 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8679482 [details]
MozReview Request: Bug 1201327 - Let DLBI detect background-position changes. r?mattwoodrow

https://reviewboard.mozilla.org/r/23429/#review20947
Attachment #8679482 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8679481 [details]
MozReview Request: Bug 1201327 - Rename mDestRect to mImageLayerDestRect. r?mattwoodrow

Bug 1201327 - Rename mDestRect to mImageLayerDestRect. r?mattwoodrow
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 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
Attachment #8679483 - Flags: review?(dbaron) → review+
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/fec3431b0b12
https://hg.mozilla.org/mozilla-central/rev/b0a00d6915bb
https://hg.mozilla.org/mozilla-central/rev/21cabf6ab3e9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Depends on: 1227327
Depends on: 1239856
Depends on: 1245255
Depends on: 1245356
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.
Flags: needinfo?(mstange)
Markus if you could clarify what needs backout here that will be super helpful!
Wes, something needs backout according to my conversation with Markus.
Flags: needinfo?(wkocher)
Adding a ni? for tomcat since he'll likely get to this sooner than I will.
Flags: needinfo?(cbook)
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.
Flags: needinfo?(mstange)
(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
Flags: needinfo?(cbook)
Flags: needinfo?(wkocher)
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?
Flags: needinfo?(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?
Flags: needinfo?(mstange)
Sure, thanks.
Markus should this get backed out of 46 as well considering the regressions? Thanks.
Flags: needinfo?(mstange)
Yes, I'll back it out on Aurora again. Should I just land the backout with a=lizzard?
Flags: needinfo?(mstange) → needinfo?(lhenry)
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?
Flags: needinfo?(lhenry) → needinfo?(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.
Flags: needinfo?(mstange)
I've backed it out on Aurora 47.
No longer depends on: 1227012
No longer depends on: 1245255
No longer depends on: 1239856
You need to log in before you can comment on or make changes to this bug.