1536781 - Consider lowering paint suppression delay on mobile

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Olli Pettay [:smaug][[e-mail]]]

On Android the delay is 250, desktop only 5ms.
Painting during load is rather different these days, we lower frame rate after first contentful paint, so hopefully smaller value is ok on mobile too -
at least initial raptor testing hints about it (but tests are noisy).

other tests
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79c53e7f577e6cf047c546653097aea101861db3

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1536781, use the same paint suppression delay on mobile and desktop, r=emilio

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

The patch reveals unrelated issue breaking one wpt test related to fragment navigation and scrolling.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

This does affect to fcp on some pages quite a lot https://i.ibb.co/zRcjwCv/image.png

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Aha, the issue is in the test, I'd say. It doesn't work too well on mobile, and with the long suppression which just don't trigger the issue while the test is running. But visually the behavior is there, since there isn't anything to scroll once page has been painted. (Mobile Chrome seems to have issues with the test anyhow)
http://mozilla.pettay.fi/moztests/fragment_nav_orig.html vs
http://mozilla.pettay.fi/moztests/fragment_nav.html

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1536781, use the same paint suppression delay on mobile and desktop, r=emilio

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a04938b3a757
use the same paint suppression delay on mobile and desktop, r=emilio

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Let's see to what all numbers this affects. I wouldn't be surprised if paint scheduling after fcp will need to be tweaked a bit.

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df1007a56ae9
Backed out changeset a04938b3a757 for android mochitest failures on test_settings_fontinflation.html.

Comment 9

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&selectedJob=235813460&revision=a04938b3a757b9141600b2dbc80267fa310c7764

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=235813460&repo=autoland&lineNumber=2002

Backout link: https://hg.mozilla.org/integration/autoland/rev/df1007a56ae9

task 2019-03-25T11:28:53.491Z] 11:28:53 INFO - 134 INFO TEST-PASS | mobile/android/tests/browser/chrome/test_settings_fontinflation.html | system font scale is enabled
[task 2019-03-25T11:28:53.491Z] 11:28:53 INFO - 135 INFO TEST-PASS | mobile/android/tests/browser/chrome/test_settings_fontinflation.html | text zoom remains at default value
[task 2019-03-25T11:28:53.491Z] 11:28:53 INFO - Buffered messages finished
[task 2019-03-25T11:28:53.491Z] 11:28:53 INFO - 136 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_settings_fontinflation.html | Snapshot canvases are not the same size: 400x559 vs. 980x1370
[task 2019-03-25T11:28:53.491Z] 11:28:53 INFO - SimpleTest.ok@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:275:18
[task 2019-03-25T11:28:53.491Z] 11:28:53 INFO - compareSnapshots@chrome://mochikit/content/tests/SimpleTest/WindowSnapshot.js:24:5
[task 2019-03-25T11:28:53.492Z] 11:28:53 INFO - assertSnapshots@chrome://mochikit/content/tests/SimpleTest/WindowSnapshot.js:61:5
[task 2019-03-25T11:28:53.492Z] 11:28:53 INFO - test_fontInflationPrecedence@chrome://mochitests/content/chrome/mobile/android/tests/browser/chrome/test_settings_fontinflation.html:159:5
[task 2019-03-25T11:28:53.496Z] 11:28:53 INFO - 137 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_settings_fontinflation.html | reftest comparison: == fontInflationOn fontScaleWithFontInflation
[task 2019-03-25T11:28:53.496Z] 11:28:53 INFO - SimpleTest.ok@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:275:18
[task 2019-03-25T11:28:53.498Z] 11:28:53 INFO - assertSnapshots@chrome://mochikit/content/tests/SimpleTest/WindowSnapshot.js:63:3
[task 2019-03-25T11:28:53.498Z] 11:28:53 INFO - test_fontInflationPrecedence@chrome://mochitests/content/chrome/mobile/android/tests/browser/chrome/test_settings_fontinflation.html:159:5
[task 2019-03-25T11:28:53.498Z] 11:28:53 INFO - 138 INFO Now waiting for load event from browser
[task 2019-03-25T11:29:03.809Z] 11:29:03 INFO - 139 INFO Received event load from browser

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

FWIW, I filed bug 1538685

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

JanH, test_settings_fontinflation.html seems to depend on the current very slow paint suppression timer.
Does the test failure ring any bells?

(I'm doing currently some tryserver-debugging, perhaps that reveals something)

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/try/rev/f974d7d7d46c0b7f79721beb13fd989743c7fe5a seems to fix the testcase.
Now making that code a bit nicer.

Comment 13

[Jan Henning [:JanH]]

No, never encountered that.

Given bug 1538685, is the viewport really ending up with a different size for good, or is that just a transient effect until the MobileViewportManager kicks in and calculates the real correct viewport size, and the changed paint suppression is just somehow causing the test code to run at a different moment relative to viewport calculation than it used to?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1536781, use the same paint suppression delay on mobile and desktop, r=emilio

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bfa4f84a030
use the same paint suppression delay on mobile and desktop, r=emilio

Comment 16

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/6bfa4f84a030

Comment 17

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16103 for changes under testing/web-platform/tests

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9053428 [details]
Bug 1536781, use the same paint suppression delay on mobile and desktop, r=emilio

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: NA
• User impact if declined: Slower first contentful paint during page load
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: I guess this gets tested anyhow while doing any testing on mobile, this is after all about page load.
• List of other uplifts needed: NA
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Changing just the timer value of paint suppression to the same what desktop has had for ages.
• String changes made/needed: NA

Comment 20

[Sorina Florean [:sflorean]]

• If yes, steps to reproduce: I guess this gets tested anyhow while doing any testing on mobile, this is after all about page load.

For testing: Just browsing and pay attention to the loading time, or we have to measure also the time compared with an affected build?

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

For QA just looking if there is something weird happening during page load, I'd say. Some weird painting issues.
Performance measurements are done by raptor tests and such.

Comment 22

[Eliza Balazs]

smaug: Hi and thank you for the quick reply!

I tested this on the latest version of Nightly 68.0a1 (2019-03-29) with Nexus 6P (Android 8.1.0), Samsung Galaxy Note 8 (Android 9), Samsung Galaxy Tab S3 (Android 8; Tablet) and I didn't see any weird painting issues during page load.
Pages tested:

• amazon.com;
• youtube.com;
• facebook.com;
• wikipedia.com;
• twitter.com.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

thanks for testing. And if in the near future something weird shows up, one can change the pref value from about:config nglayout.initialpaint.delay
It used to be 250 on android but is now 5.

Comment 24

[Pascal Chevrel:pascalc (PTO until April 26)]

Olli, this bug is a P3, is there a compelling reason to have it in 67 and not let it ride the trains?

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Just because it affects to first contentful paint on Android so much.
I don't have too strong opinion.

Comment 26

[Chris Peterson [:cpeterson]]

(In reply to Pascal Chevrel:pascalc from comment #24)

Olli, this bug is a P3, is there a compelling reason to have it in 67 and not let it ride the trains?

67=fix-optional. Neither Fenix MVP nor Firefox for Fire TV will use GeckoView 67, so we don't need to uplift this fix to 67 Beta unless you want it in Fennec 67.

Comment 27

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 28

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9053428 [details]
Bug 1536781, use the same paint suppression delay on mobile and desktop, r=emilio

Given that we are no longer targeting 67 for Geckoview, I think that this can ride the trains to 68, thanks.

Comment 29

[Chris Peterson [:cpeterson]]

(In reply to Pascal Chevrel:pascalc from comment #28)

Given that we are no longer targeting 67 for Geckoview, I think that this can ride the trains to 68, thanks.

OK. 67=wontfix