1294625 - Try to tick refresh driver during html parsing

Status: NEW

(Core :: Layout, defect, P3)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

When we have a big document with a lot of js script, gecko will break the parsing[1] and schedule the remaining parser later. That could improve the response for user activity.
But if the main thread message queue is full of message, the refresh driver tick might be handled later.
I would like to tick the refresh driver directly for every parsing breaking. Maybe the user could see the webpage sooner.

[1]
https://hg.mozilla.org/mozilla-central/annotate/0502bd9e025edde29777ba1de4280f9b52af4663/parser/html/nsHtml5TreeOpExecutor.cpp#l498

Comment 1

[Boris Zbarsky [:bzbarsky]]

As long as we don't start layout unnecessarily and cause FOUC, this might be reasonable....  Also note that there is a good chance this would violate the spec for rAF, which is supposed to come from vsync.

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [wip] adjust refresh driver tick.

Comment 3

[Thinker Li [:sinker]]

Hi Shako, could you help Jerry to do tests for comparison?

Comment 4

[Shako Ho]

Hi Thinker,

Could you ask Jerry to push this patch to try server and provide the user name and build hash for me?

After I got those information, we will do the following test.

Comment 5

[Thinker Li [:sinker]]

Jerry, I think that PresShell::Paint() should be called in the refresh driver when your change kicks it.  But, I don't see it.  Could you check it?

Comment 6

[Thinker Li [:sinker]]

With Jerry's patch and disable PresShell::mPaintingSuppressed, it improves a lot on my laptop.  The time to showing the table has been cut down to 11s from 18s with m-c debug build.

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [wip] adjust refresh driver tick. v2

Comment 8

[Hiroyuki Ikezoe (:hiro)]

As bz noted in comment 1, I suspect this will break browser processing model.

https://html.spec.whatwg.org/multipage/webappapis.html#processing-model-8

Comment 9

[Thinker Li [:sinker]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> As bz noted in comment 1, I suspect this will break browser processing model.
> 
> https://html.spec.whatwg.org/multipage/webappapis.html#processing-model-8

As a reminder!

Right, I think Jerry has that in mind.  And, this is a in-progress task.  We need to try a lot of POC patches here, even it is buggy now, to help us figure out causes, solutions and how would it help.

It would be good and welcomed to put down all concerns here.  But, don't be worry about it!  All POC patches would be re-worked for several times before they go to the review process.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Right, I don't think it's a dealbreaker to need to go back to the spec and ask for changes; just something to keep in mind.

That said, timing things in debug builds is pretty pointless.  The results often differ significantly from opt builds qualitatively; it's not uncommon for a change to make debug builds faster while making opt build slower or vice versa.

Comment 11

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Shako Ho from comment #4)
> Hi Thinker,
> 
> Could you ask Jerry to push this patch to try server and provide the user
> name and build hash for me?
> 
> After I got those information, we will do the following test.

I'm not sure the test plan, but there are three build could be tested.

1. check painting suppression status asap(bug 1271691) + tick refresh driver during html parsing(bug 1294625)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1f7ca95b09f

2. tick refresh driver during html parsing(bug 1294625)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0063015b7218

3. check painting suppression status asap(bug 1271691)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e14bdefbe9a1

Comment 12

[Shako Ho]

Ok, I will use Hasal framework to test these three builds.
Will update to you the result after the test finish.

Comment 13

[Shako Ho]

Hi Jerry,

From our testing, we didn't see too much difference between different build, the detail test result as below: 
1. check painting suppression status asap(bug 1271691) + tick refresh driver during html parsing(bug 1294625)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1f7ca95b09f

Median time: 15583.3333333
Avg time   : 15624.8148148
Std dev    : 417.928295712   

2. tick refresh driver during html parsing(bug 1294625)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0063015b7218

Median time: 15650.0                       
Avg time   : 15637.037037
Std dev    : 470.778475627    

3. check painting suppression status asap(bug 1271691)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e14bdefbe9a1

Median time: 15638.8888889                
Avg time   : 15605.5555556          
Std dev    : 449.94512697

Comment 14

[Thinker Li [:sinker]]

Shako, could you run the test that measure the time to show just content?

Comment 15

[Shako Ho]

Sure, will update the status after test complete

Comment 16

[Shako Ho]

Hi Thinker,

The test result as below:

1. check painting suppression status asap(bug 1271691) + tick refresh driver during html parsing(bug 1294625)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1f7ca95b09f

Median time: 4722.22222222      
Avg time   : 4733.7037037       
Std dev    : 165.812213019

2. tick refresh driver during html parsing(bug 1294625)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0063015b7218

Median time: 4877.77777778      
Avg time   : 5278.51851852      
Std dev    : 730.825866529

3. check painting suppression status asap(bug 1271691)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e14bdefbe9a1

Median time: 4500.0                           
Avg time   : 4512.59259259      
Std dev    : 89.8894306321

Comment 17

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

FWIW, bug 1306591 should have helped at least in e10s case.

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:dholbert, could you have a look please?
For more information, please visit auto_nag documentation.