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

Status: VERIFIED FIXED

(Core :: Web Painting, defect, P1)

Description

[Bas Schouten (:bas.schouten)]

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 1

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1471437 - Part 1: Store the previous paint rect before occlusion culling.

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

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1471437 - Part 2: Reset mPaintRect only when the new building rect is different, and update mPaintRectValid when it is.

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

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8988031 [details]
Bug 1471437 - Part 1: Store the previous paint rect before occlusion culling.

https://reviewboard.mozilla.org/r/253282/#review259904

Comment 4

[Matt Woodrow (:mattwoodrow)]

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7758cd890fa8
https://hg.mozilla.org/mozilla-central/rev/ebe99e26ec86

Comment 7

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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?

Comment 8

[Bas Schouten (:bas.schouten)]

(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.

Comment 9

[ovidiu boca[:Ovidiu]]

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.

Comment 10

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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?

Comment 11

[Bas Schouten (:bas.schouten)]

(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 12

[Bas Schouten (:bas.schouten)]

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

Comment 13

[Bas Schouten (:bas.schouten)]

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

Comment 14

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7c3840f0769f
https://hg.mozilla.org/releases/mozilla-beta/rev/acc7133a29fb

Comment 16

[ovidiu boca[:Ovidiu]]

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.