Bug 54108

Summary:

Layout stall waiting for external scripts to load

Product:

WebKit

Reporter:

Silvio <silvio.ventres>

Component:

Page Loading

Assignee:

Nobody <webkit-unassigned>

Status:

VERIFIED REMIND

Severity:

Normal

CC:

ap, koivisto, peter, silvio.ventres, tonyg

Priority:

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-

Silvio

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

Attachments
patch to check domain origin from within HTMLScriptRunner::runScript() (1.47 KB, patch) 2011-02-09 10:25 PST, Silvio	no flags	Details Formatted Diff Diff
patch to check domain origin from within HTMLScriptRunner::runScript() -- updated to support bugzilla's diff parser (1.39 KB, patch) 2011-02-09 10:29 PST, Silvio	no flags	Details Formatted Diff Diff
Makes initial layout/paint appear sooner by prioritizing scripts by domain-origin (compiled and tested working) (1.37 KB, patch) 2011-02-12 07:35 PST, Silvio	tonyg: review-	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Silvio

Comment 1 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(..) ---------------------

Silvio

Comment 2 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?

Silvio

Comment 3 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.

Silvio

Comment 4 2011-02-09 10:25:40 PST

Created attachment 81830 [details] patch to check domain origin from within HTMLScriptRunner::runScript() not compiled yet

Silvio

Comment 5 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

Silvio

Comment 6 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

Silvio

Comment 7 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)

Silvio

Comment 8 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.

Silvio

Comment 9 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>.

Silvio

Comment 10 2011-02-12 07:50:20 PST

Marked difference on sites like http://technorati.com Wow..

Tony Gentilcore

Comment 11 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.

Silvio

Comment 12 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..

Silvio

Comment 13 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.

Silvio

Comment 14 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.

Peter Beverloo

Comment 15 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.

Tony Gentilcore

Comment 16 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.

Silvio

Comment 17 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

Silvio

Comment 18 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.

Note You need to log in before you can comment on or make changes to this bug.