Closed Bug 1471437 Opened 3 years ago Closed 3 years ago

RecomputeVisibility can often be wrongly skipped for items outside the painted region

Categories

(Core :: Web Painting, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox61 --- unaffected
firefox62 + verified
firefox63 + verified

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

Details

(Keywords: regression)

Attachments

(2 files)

This does not affect correctness, but it has a big impact on performance, since we now spend time painting items which are outside the area actually being drawn.

There's two bugs:

1. We store the current visible region as the old visible region after having done occlusion culling on the current visible region, since the background is opaque, that means it's always empty. It will therefor assume a display item's paint rect must have always been empty the previous frame.
2. If SetBuildingRect is called, the paintrect is reset, however mPaintRectValid is not reset.
Comment on attachment 8988031 [details]
Bug 1471437 - Part 1: Store the previous paint rect before occlusion culling.

https://reviewboard.mozilla.org/r/253282/#review259904
Attachment #8988031 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8988041 [details]
Bug 1471437 - Part 2: Reset mPaintRect only when the new building rect is different, and update mPaintRectValid when it is.

https://reviewboard.mozilla.org/r/253288/#review259906
Attachment #8988041 - Flags: review?(matt.woodrow) → review+
Priority: -- → P1
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7758cd890fa8
Part 1: Store the previous paint rect before occlusion culling. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebe99e26ec86
Part 2: Reset mPaintRect only when the new building rect is different, and update mPaintRectValid when it is. r=mattwoodrow
Summary: RecomputerVisibility can often be wrongly skipped for items outside the painted region → RecomputeVisibility can often be wrongly skipped for items outside the painted region
https://hg.mozilla.org/mozilla-central/rev/7758cd890fa8
https://hg.mozilla.org/mozilla-central/rev/ebe99e26ec86
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Sounds like a good opportunity to improve performance. Do you want to request uplift to beta? Is there a good way we can verify the fix in nightly?
Flags: needinfo?(bas)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #7)
> Sounds like a good opportunity to improve performance. Do you want to
> request uplift to beta? Is there a good way we can verify the fix in nightly?

Other than profiling or a debugger there's not really a way to verify this on your average page. By using a test page where the performance issue is pathological (https://mattwoodrow.github.io/dl-test/dl-test.html?layer=flattened) you should be able to see a very clear performance difference between before/after the fix.
Flags: needinfo?(bas)
I verified this issue on Mac OS X 10.12, Windows 10 and Ubuntu 16.04 using FF Nightly 63.0a1(2018-07-02) and I can confirm the fix. I was also able to reproduce the issue using the link from the comment 8 with a build before the fix.
I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
I'm ok with letting this fix ride with 63. If you think this should make it into 62, please request uplift.
Bas, I also forgot to ask is this a new issue, or newly exposed in 62 from any recent work?
Flags: needinfo?(bas)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #10)
> I'm ok with letting this fix ride with 63. If you think this should make it
> into 62, please request uplift.
> Bas, I also forgot to ask is this a new issue, or newly exposed in 62 from
> any recent work?

Yes, this is a new issue, we should probably uplift it. I'll add a request.
Comment on attachment 8988031 [details]
Bug 1471437 - Part 1: Store the previous paint rect before occlusion culling.

Approval Request Comment
[Feature/Bug causing the regression]: 1460491
[User impact if declined]: Perf issues with small invalidations
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Other patches in this bug
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: Small, well understood fix.
[String changes made/needed]: None
Flags: needinfo?(bas)
Attachment #8988031 - Flags: approval-mozilla-beta?
Comment on attachment 8988041 [details]
Bug 1471437 - Part 2: Reset mPaintRect only when the new building rect is different, and update mPaintRectValid when it is.

Approval Request Comment
[Feature/Bug causing the regression]: 1460491
[User impact if declined]: Perf issues with small invalidations
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Other patches in this bug
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: Small, well understood fix.
[String changes made/needed]: None
Attachment #8988041 - Flags: approval-mozilla-beta?
Comment on attachment 8988031 [details]
Bug 1471437 - Part 1: Store the previous paint rect before occlusion culling.

Fix for perf regression from 62, let's uplift for beta 12 later this week.
Attachment #8988031 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8988041 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I verified this issue on FF beta 62.0b12 on Windows 10, Mac OS X 10.12 and Ubuntu 16.04 using the test case from comment 8 and I can confirm the fix.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.