Bug 54108

Summary: Layout stall waiting for external scripts to load
Product: WebKit Reporter: Silvio <silvio.ventres>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: VERIFIED REMIND    
Severity: Normal CC: ap, koivisto, peter, silvio.ventres, tonyg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://solid.eqoppa.com/testlag2.html
Attachments:
Description Flags
patch to check domain origin from within HTMLScriptRunner::runScript()
none
patch to check domain origin from within HTMLScriptRunner::runScript() -- updated to support bugzilla's diff parser
none
Makes initial layout/paint appear sooner by prioritizing scripts by domain-origin (compiled and tested working) tonyg: review-

Description Silvio 2011-02-09 08:48:55 PST
When there is a blocking script/css link in <head> or beginning of <body>, the engine stalls until the link is resolved.
Given the prevalence of non critical slow-loading cross-domain scripts, such as ads and analytics trackers, a heuristic that
prioritizes same-origin css/scripts over cross-domain ones will possibly allow the initial layout/paint event to happen
earlier, instead of showing a blank page while stalling.

Possible/probable adverse side effect: on pages with dependencies bound to the deprioritized scripts, in worst case, a full reparse might be needed.
To weigh the performance impact, some testing is needed.
Comment 1 Silvio 2011-02-09 09:07:52 PST
A draft domain-comparison procedure for new url, to be inserted in CachedResourceLoader::preload():


-- unchecked-draft --

newHost = url.host().isNull() ? "" : url.host();
docHost = m_document->securityOrigin()->host();

//find the second dot or beginning of the String
newHostDomain = newHost.substr(newHost.substr(0,newHost.find_last_of('.')).find_last_of('.')+1);
docHostDomain = docHost.substr(docHost.substr(0,docHost.find_last_of('.')).find_last_of('.')+1);

if(equalIgnoringCase(docHostDomain,newHostDomain))
  requestPreload(..)
else
  m_pendingPreloads.append(..)

---------------------
Comment 2 Silvio 2011-02-09 09:16:57 PST
Seems like simply deferring the preload might not help at all if the engine is already blocking waiting for this resource. Can someone please point to where the block can be mitigated?
Comment 3 Silvio 2011-02-09 09:35:43 PST
Maybe a better place to check for script origin is in HTMLScriptRunner::runScript() - handle external-domain scripts as if they were deferred.
Comment 4 Silvio 2011-02-09 10:25:40 PST
Created attachment 81830 [details]
patch to check domain origin from within HTMLScriptRunner::runScript()

not compiled yet
Comment 5 Silvio 2011-02-09 10:29:54 PST
Created attachment 81832 [details]
patch to check domain origin from within HTMLScriptRunner::runScript() -- updated to support bugzilla's diff parser

not compile-checked
Comment 6 Silvio 2011-02-12 07:34:12 PST
Comment on attachment 81832 [details]
patch to check domain origin from within HTMLScriptRunner::runScript() -- updated to support bugzilla's diff parser

used String type instead of WTF::String
Comment 7 Silvio 2011-02-12 07:35:34 PST
Created attachment 82234 [details]
Makes initial layout/paint appear sooner by prioritizing scripts by domain-origin (compiled and tested working)
Comment 8 Silvio 2011-02-12 07:37:16 PST
Compiled and built webkit with the latest patch: on http://solid.eqoppa.com/testlag2.html, Initial paint happens at about 30th second vs 80th second.

The initial delay is because the script only prioritizes javascript, and css is still blocking.
Comment 9 Silvio 2011-02-12 07:46:34 PST
Seems on http://solid.eqoppa.com/testlag4.html there is no difference.

Thus, for now conclusion: patch only makes difference, and prioritizes scripts in <body>.
Nothing changes if the reference is made from within <head>.
Comment 10 Silvio 2011-02-12 07:50:20 PST
Marked difference on sites like http://technorati.com

Wow..
Comment 11 Tony Gentilcore 2011-02-12 10:06:53 PST
Comment on attachment 82234 [details]
Makes initial layout/paint appear sooner by prioritizing scripts by domain-origin (compiled and tested working)

View in context: https://bugs.webkit.org/attachment.cgi?id=82234&action=review

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:316
> +                    requestDeferredScript(script);

Yes, deferred scripts can be much faster than parsing blocking ones, so I'm not surprised you are seeing big performance wins here. But It isn't correct to treat a parsing blocking script as a deferred script. This would be a violation of the HTML5 spec and lead to broken sites because the execution order changes. The correct approach here is not a webkit change, but rather to evangelize that web sites use deferred or async scripts if the order they execute is not important.
Comment 12 Silvio 2011-02-12 12:09:50 PST
Yes, you are correct in that it might cause broken layouts.
But, given the current situation, where 90% of the web developers don't have much clue regarding the async/defer and don't want to break anything by, f.e., deferring an ad script, the heuristic works surprisingly well.

What happens when a heuristically-deferred script cause a layout problem? Can the whole page be rerendered from null, using the already-cached resources?
Then adding a counter to see amount of such events during normal operation will allow to judge the actual impact of the heuristics.

You as well as anyone know that pushing responsibility to web developers won't cause any actual change. And even if they use it, there will be ones that break their own websites by using it. The percentage will probably be much higher than the amount of errors in layout caused by this patch. 

And you will have to add "quirks" to ignore those wrongly-added asyncs/defers..
Comment 13 Silvio 2011-02-12 13:06:40 PST
Since any, especially domain-external, host latency is a dynamic metric, the user has a very limited way of guessing what it will be at time of page view.
Thus, even though some resource might _not_ have the "defer/async" attribute, the user will not be there at time of need to add it, saving the reader from frustration.
From point of view of developer, everything should be "deferred" - the more it helps interactivity the better. Tracking dependencies is not an easy user-oriented task, and, if all hosting works fast, no problems should emerge.

Forcing users to add defer/async "where needed" will make them add it everywhere to get higher performance in 80% cases, getting you into the same situation as now, but with all scripts starting with "<script defer" instead of just "<script". Why not preempt that ? The browser is a better place to make the call: "host is slow. should show something and possibly rerender, or show nothing and wait?".

If you want to force web developers to add anything, add a "strict" attribute.
The developers will know that it's use might slow down the initial paint, and use it sparingly.
Comment 14 Silvio 2011-02-12 13:13:45 PST
(In reply to comment #11)
> (From update of attachment 82234 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82234&action=review
> > Source/WebCore/html/parser/HTMLScriptRunner.cpp:316
> > +                    requestDeferredScript(script);
> Yes, deferred scripts can be much faster than parsing blocking ones, so I'm not surprised you are seeing big performance wins here. But It isn't correct to treat a parsing blocking script as a deferred script. This would be a violation of the HTML5 spec and lead to broken sites because the execution order changes. The correct approach here is not a webkit change, but rather to evangelize that web sites use deferred or async scripts if the order they execute is not important.

It's not just a "parsing blocking script", but it's a "parsing blocking script from an external domain". This is the main argument for deferring it.
Comment 15 Peter Beverloo 2011-02-12 14:04:43 PST
This patch would break any site using scripts and libraries, such as jQuery, from a content delivery network, which often is considered a best practice. Introducing new attributes or "strict modes" for HTML should be done at either the W3C or the WHATWG.
Comment 16 Tony Gentilcore 2011-02-12 14:13:14 PST
Another way to pursue this would be to write an extension that rewrites pages in unsafe ways to speed up a majority of sites and break some (although I'm not sure who would use it). But it isn't worth discussing this change in WebKit.

If you are interested in WebKit contribution, my recommendation is to search the bug database for site compatibility problems which have reductions. Work to understand and fix a few of these. You'll start to learn more about WebKit and get into a better position to think about changes like this.
Comment 17 Silvio 2011-02-12 15:06:49 PST
Will test it on more websites, including ones that use CDN js (jquery etc).
With CDN's, the whole point is that you load the script once, and then use the cached copy for all the websites that load it. So even if you miss once and it's deferred, it won't matter afterwards since it'll be cached.

Anyway, expecting little if any errors on CDN's, will report how testing went and which sites were surveyed.
If anyone has a list of websites to test, please let know.

Regarding the CDN hosting in general, it's not as "best practice" by far: http://zoompf.com/blog/2010/01/should-you-use-javascript-library-cdns
Comment 18 Silvio 2011-02-23 13:05:44 PST
Verified the solution on various websites: it works with no problems with most websites. The only problematic ones are youtube.com and sites which embed youtube movies. Need to check whether it's because of the patch or because of the problems in the current svn code or local flash configuration.

Regarding the general domain-priority patching, a similar solution for image loading can be used to push external-source image/media resources to the end of the queue, making sure tracking-pixels/ads/flash from slow hosts are not making the web browsing miserable.

In any case, here is another proposal, not having to do with domain heuristics:
-
Since the current solution adds a chance of the page not rendering correctly in the end, here's a proposal for a different procedure, which guarantees compliant render output for almost all scripts except ones using document.write.

Instead of deferring externally-hosted scripts, all scripts are considered deferred, and the browser then resolves dependencies via a dependency resolution mechanism:

For each completely loaded script resource:
1. Analyze for function calls and definitions as usual.
2. If all functions are well-defined, the script is executed immediately.
3. Resolve any functions in the "unresolved functions" table using the definitions in the currently-loaded script, and decrement functions_to_be_resolved_until_execution for the scripts using those functions.
4. Schedule any scripts with functions_to_be_resolved_until_execution==0 to run next in queue, and remove them from the awaiting_function_resolution table
5. If undefined function calls are encountered, the function symbols are added into the "unresolved functions" table, and the script is linked to them. Script reference is added to a separate awaiting_function_resolution table with a functions_to_be_resolved_until_execution value specifying the amount of functions to be resolved for script to run, f.e. "8". The script is _not_ executed.

This can also be done with DOM-snapshot rollback mechanism or multithreaded script execution which would probably have lots of race conditions.