Bug 208896 - Defer async scripts until DOMContentLoaded or first paint, whichever comes first
Summary: Defer async scripts until DOMContentLoaded or first paint, whichever comes first
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 207698
  Show dependency treegraph
 
Reported: 2020-03-10 16:59 PDT by Chris Dumez
Modified: 2020-03-16 08:29 PDT (History)
10 users (show)

See Also:


Attachments
WIP Patch (3.19 KB, patch)
2020-03-10 17:49 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (7.18 KB, patch)
2020-03-11 07:42 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (7.95 KB, patch)
2020-03-11 08:10 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-03-10 16:59:10 PDT
Defer async scripts until DOMContentLoaded or first paint, whichever comes first. In Bug 207698, we deferred them until DOMContentLoaded, as a first-paint optimization. However, this seems overly aggressive on pages like wikipedia and it is sufficient to defer those scripts until first-paint to get the performance win.
Comment 1 Chris Dumez 2020-03-10 17:49:55 PDT
Created attachment 393194 [details]
WIP Patch
Comment 2 Chris Dumez 2020-03-11 07:42:37 PDT
Created attachment 393239 [details]
Patch
Comment 3 Chris Dumez 2020-03-11 08:10:09 PDT
Created attachment 393244 [details]
Patch
Comment 4 WebKit Commit Bot 2020-03-11 10:30:55 PDT
Comment on attachment 393244 [details]
Patch

Clearing flags on attachment: 393244

Committed r258268: <https://trac.webkit.org/changeset/258268>
Comment 5 WebKit Commit Bot 2020-03-11 10:30:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2020-03-11 10:31:14 PDT
<rdar://problem/60329406>
Comment 7 krinklemail 2020-03-12 14:15:59 PDT
Hi Chris,

Thanks for the quick action, I really appreciate it!

I'm still very new to the WebKit source code, and am trying to understand how the first paint event "notifies" informs the pending async script to start execution.

For DOMContentLoaded, I see that the the ScriptRunner is receives this notice from Document.cpp in its documentFinishedParsing() method. I don't see something similar for first paint that would similarly clear the "don't execute yet" flag.

Maybe it is that the shouldDeferAsynchronousScriptsUntilParsingFinishes condition is automatically re-evaluated after important events such as FP and DCL, but I couldn't find it (and if so, perhaps documentFinishedParsing is redundant in that case?).
Comment 8 Chris Dumez 2020-03-16 08:12:41 PDT
(In reply to krinklemail from comment #7)
> Hi Chris,
> 
> Thanks for the quick action, I really appreciate it!
> 
> I'm still very new to the WebKit source code, and am trying to understand
> how the first paint event "notifies" informs the pending async script to
> start execution.
> 
> For DOMContentLoaded, I see that the the ScriptRunner is receives this
> notice from Document.cpp in its documentFinishedParsing() method. I don't
> see something similar for first paint that would similarly clear the "don't
> execute yet" flag.
> 
> Maybe it is that the shouldDeferAsynchronousScriptsUntilParsingFinishes
> condition is automatically re-evaluated after important events such as FP
> and DCL, but I couldn't find it (and if so, perhaps documentFinishedParsing
> is redundant in that case?).

You are right that async scripts may still be delayed longer than they should after first paint. I am working on evaluating performance if we hook up the script runner to the first paint.
Comment 9 Chris Dumez 2020-03-16 08:29:14 PDT
(In reply to Chris Dumez from comment #8)
> (In reply to krinklemail from comment #7)
> > Hi Chris,
> > 
> > Thanks for the quick action, I really appreciate it!
> > 
> > I'm still very new to the WebKit source code, and am trying to understand
> > how the first paint event "notifies" informs the pending async script to
> > start execution.
> > 
> > For DOMContentLoaded, I see that the the ScriptRunner is receives this
> > notice from Document.cpp in its documentFinishedParsing() method. I don't
> > see something similar for first paint that would similarly clear the "don't
> > execute yet" flag.
> > 
> > Maybe it is that the shouldDeferAsynchronousScriptsUntilParsingFinishes
> > condition is automatically re-evaluated after important events such as FP
> > and DCL, but I couldn't find it (and if so, perhaps documentFinishedParsing
> > is redundant in that case?).
> 
> You are right that async scripts may still be delayed longer than they
> should after first paint. I am working on evaluating performance if we hook
> up the script runner to the first paint.

I believe the current behavior is that async scripts would run whenever the parser yields after the first paint has happened. It is true that it is not exactly after first paint and that those async scripts may be delayed longer.

This is not an obviously bad behavior but I will compare this and running the async scripts right after first paint on our benchmarks.