Deferred script loading v2 #3696

michaelletzgus · 2017-03-03T14:59:34Z

Slightly improved version of #3656

Problem before this PR:

<script> loading was performed asynchronously, loading 50-100 external js scripts an executing them sequentially was slow.

Example:
Full reload of "Files" app with almost all AddOns activated lasts 12.1 sec, loading 125 external .js.

Full discussion: #2272

Fix provided by this PR:

Script loading is done deferred with <script defer>.
That means:

Script download starts as soon as the parser reaches the script tag
Scripts will be downloaded simultaneously
Script execution is performed in order(!) after parsing is finished

Modifications:

Templates:
Placed loading of all external resources close together without any interruption by inline script or other code. The inline script code is placed last in the document - but will be executed first(!) because all other external script are deferred (see above).

Unnecessary type= has been removed from inline <script>

template.php:
<script> tags injected by AddOns take the 'defer' attributes here, but only when src attribute is present, otherwise defer is not allowed.

<script defer>:
https://www.w3schools.com/tags/att_script_defer.asp
https://developer.mozilla.org/de/docs/Web/HTML/Element/script
https://varvy.com/pagespeed/defer-loading-javascript.html
http://www.growingwiththeweb.com/2014/02/async-vs-defer-attributes.html
http://peter.sh/experiments/asynchronous-and-deferred-javascript-execution-explained/
https://www.html5rocks.com/en/tutorials/speed/script-loading/

New behavior:
Full (re)loading of NC webpages is now 3-10 times faster, depending on link latency and the underlying protocol (http1, http2) .
When using http2 (or http1 pipelining...) huge amounts of external resource should not be a problem any more.

=> Full reload of "Files" app with almost all AddOns activated now lasts 2.3 sec, loading 125 external .js.

Possible drawback of this solution:
IE<=9 does not support defer properly, order of execution is NOT guaranteed. This may cause problems with script dependencies!

mention-bot · 2017-03-03T14:59:36Z

@michaelletzgus, thanks for your PR! By analyzing the history of the files in this pull request, we identified @LukasReschke, @nickvergessen and @ChristophWurst to be potential reviewers.

nickvergessen · 2017-03-06T10:20:39Z

core/templates/layout.base.php

 		<?php if (isset($_['inline_ocjs'])): ?>
-			<script nonce="<?php p(\OC::$server->getContentSecurityPolicyNonceManager()->getNonce()) ?>" type="text/javascript">
+			<script nonce="<?php p(\OC::$server->getContentSecurityPolicyNonceManager()->getNonce()) ?>">


is this change required?

I don't know, for me both version work faster.
But: HTML5 does not require the type attribute. My intention was to prevent current or future browsers triggering any kind of legacy or compatibility mode which might interrupt the concurrent resource loading.

In short: The change is possibly not required for defeating the performance problem...

ChristophWurst · 2017-03-06T14:27:27Z

Wow! This is amazing. The page loads significantly faster. Thank you very much 😍

ChristophWurst · 2017-03-06T14:29:03Z

core/templates/layout.base.php

@@ -18,15 +18,15 @@
 		<?php foreach($_['printcssfiles'] as $cssfile): ?>
 			<link rel="stylesheet" href="<?php print_unescaped($cssfile); ?>" media="print">
 		<?php endforeach; ?>
+		<?php foreach ($_['jsfiles'] as $jsfile): ?>


do we really have to move those lines up? This could potentially change/break the loading order with the scripts below (inline_ocjs)

Good point, it changed the order. inline_ocjs will be executed first (while parsing), the deferred scripts after parsing but before the DOMContentLoaded event comes up.

The standard states: "The defer attribute is only for external scripts (should only be used if the src attribute is present)."

I have found no code in the inline_ocjs which depends on other.
If the old order of execution (first inline, then extern) is desired the inline script code must become external. So why do we have internal code here at all? I think all these variables could be loaded form extern as well.

@ChristophWurst
In fact this does NOT change the order of execution when using deferfor the external script, it only allows Firefox to download css and js in parallel.
With the inline_js block placed between css and js downloading of css and js happens synchronously. This seems to be a "feature" of at least Firefox...

I see, thanks for the explanation :-)

ChristophWurst · 2017-03-06T14:29:22Z

core/templates/layout.guest.php

@@ -19,15 +19,15 @@
 		<?php foreach($_['printcssfiles'] as $cssfile): ?>
 			<link rel="stylesheet" href="<?php print_unescaped($cssfile); ?>" media="print">
 		<?php endforeach; ?>
+		<?php foreach($_['jsfiles'] as $jsfile): ?>


same problem/question here

ChristophWurst · 2017-03-06T14:29:29Z

core/templates/layout.user.php

@@ -26,15 +26,15 @@
 		<?php foreach($_['printcssfiles'] as $cssfile): ?>
 			<link rel="stylesheet" href="<?php print_unescaped($cssfile); ?>" media="print">
 		<?php endforeach; ?>
+		<?php foreach($_['jsfiles'] as $jsfile): ?>


ChristophWurst · 2017-03-06T14:30:39Z

lib/private/legacy/template.php

@@ -233,6 +233,9 @@ public function fetchPage($additionalParams = null) {
 			$headers = '';
 			foreach(OC_Util::$headers as $header) {
 				$headers .= '<'.\OCP\Util::sanitizeHTML($header['tag']);
+				if ( strcasecmp($header['tag'], 'script') == 0 and in_array('src', array_map('strtolower', array_keys($header['attributes']))) ) {


use === for comparison and && as logical and

benediktg · 2017-03-07T15:59:18Z

I would recommend to eliminate the commits e3a4e4c, 49b6c4e and 9207852 because I think it's nonsense to merge master into this feature branch. ;)

MorrisJobke · 2017-03-08T04:13:35Z

I would recommend to eliminate the commits e3a4e4c, 49b6c4e and 9207852 because I think it's nonsense to merge master into this feature branch. ;)

@michaelletzgus A rebase will automatically remove them ;)

nickvergessen · 2017-03-08T08:23:30Z

@michaelletzgus you don't need to close your PR, you can do a rebase easily on the same PR:

git checkout master
git pull nextcloud master
git checkout deferred-script-loading-v2
git rebase -i nextcloud/master
git push -f origin deferred-script-loading-v2

michaelletzgus · 2017-03-08T08:55:44Z

Sorry for the confusion, but git will definitively not my friend, I'm grown up with CVS&SVN ;-)
The rebase gave me some nice error messages - before git killed my test instance (sqlite db, config.php) and removed all my file changes.

nickvergessen · 2017-03-08T09:12:46Z

Sorry for that, thanks anyway for trying and it seems to have worked 🎉

michaelletzgus · 2017-03-08T09:14:50Z

@nickvergessen No problem, maybe git will be my friend in a few days... ;)

MorrisJobke · 2017-03-08T17:47:35Z

@nickvergessen No problem, maybe git will be my friend in a few days... ;)

@michaelletzgus Give https://try.github.io a try 😉 It helps to understand the concepts :)

mm0 · 2017-03-14T09:25:58Z

Can someone explain what caused this to break? I have an installation of 11.0.1 that doesn't have the same behavior as 11.0.2. Somehow 11.0.1 did not need the defer tag, yet it loads quickly.

michaelletzgus · 2017-03-14T11:57:46Z

I observed no difference between 11.0.1 and 11.0.2 when using Firefox, both are "slow" without defer.

michaelletzgus · 2017-03-15T13:59:10Z

@nickvergessen @MorrisJobke
What to do next? Waiting for the Firefox people?

nickvergessen · 2017-03-16T13:01:41Z

When it doesn't get slower than it is at the moment, I'm fine with merging it

1. Remove type attribute from inline_js 2. Add defer attribute to external script 3. Move inline_script down Date: Wed Mar 8 09:43:51 2017 +0100 Committer: Michael Letzgus <michaelletzgus@users.noreply.github.com> Signed-off-by: Michael Letzgus <michaelletzgus@users.noreply.github.com>

codecov-io · 2017-03-18T11:56:47Z

Codecov Report

Merging #3696 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #3696      +/-   ##
============================================
- Coverage     54.24%   54.24%   -0.01%     
- Complexity    21112    21114       +2     
============================================
  Files          1304     1304              
  Lines         80655    80657       +2     
  Branches       1275     1275              
============================================
  Hits          43752    43752              
- Misses        36903    36905       +2

Impacted Files	Coverage Δ	Complexity Δ
core/templates/layout.guest.php	`0% <ø> (ø)`	`0 <0> (ø)`	❌
core/templates/layout.base.php	`0% <ø> (ø)`	`0 <0> (ø)`	❌
core/templates/layout.user.php	`0% <ø> (ø)`	`0 <0> (ø)`	❌
lib/private/legacy/template.php	`40.74% <0%> (-0.51%)`	`37 <0> (+2)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1febf04...a39b996. Read the comment docs.

Bubu · 2017-03-18T14:52:48Z

Tested and works, applied on top of 11.0.2.

This is marked for version 12, any chance this can be applied for 11.0.3? It is really a difference of night and day with and without this patch.

LukasReschke · 2017-03-20T12:10:34Z

This is marked for version 12, any chance this can be applied for 11.0.3? It is really a difference of night and day with and without this patch.

No. We do only backport critical patches. This here has the potential to breaking stuff and I'm still keen on feedback at https://bugzilla.mozilla.org/show_bug.cgi?id=1342781

michaelletzgus · 2017-03-20T19:45:55Z

I've found the first problem with defer in app "files_markdown" (https://github.com/icewind1991/files_markdown/blob/master/js/editor.js#L23).
I think this is not specific to this app but might be a more fundamental problem, maybe with jQuery, see below.
jQuery .then() in line 23 is fired to early:

   $.when(
                OCA.Files_Markdown.Preview.loadMarked(),
                OCA.Files_Markdown.Preview.loadHighlight(),
                OCA.Files_Markdown.Preview.loadKaTeX()
        ).then(function () {
                this.renderer = new marked.Renderer();

This results in "ReferenceError: marked is not defined".

Problem seems to be related to this:
jquery/jquery#3271
http://stackoverflow.com/questions/37272020/jquery-loaded-async-and-ready-function-not-working

xdmx · 2017-03-21T16:51:24Z

I've applied these changes to my nextcloud 11.0.0 and the difference of loading is a lot.

After few days of usage I've noticed that my external calendars (for example webcal://www.calendarlabs.com/templates/ical/International-Holidays.ics) aren't loading anymore.

Checking the console it says:

Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src 'nonce-Qug1WTdoeFpiZGgzS1g3cGxpMmNnV5B0WU15c05pdUI0eGZGODBEcEhLbz06Y1NZcHZrMHFYK3NWUWoraDhoVDF8MXVyRXZUNmVIelqxbnlIdXdxTlR2OD0=' 'unsafe-eval'”). Source: ;(function installGlobalHook(window) {
 ....

I haven't tried this under the v11.0.2

nickvergessen · 2017-03-23T15:53:22Z

Works fine here on 12. (with the very same url webcal://www.calendarlabs.com/templates/ical/International-Holidays.ics)
So I'd say either try 12 and open a new issue when it still happens there.

LukasReschke

Just commenting here so that nobody merges this. I don't feel entirely confident here.

This breaks some apps as some people pointed out.
I still am curious about a feedback at https://bugzilla.mozilla.org/show_bug.cgi?id=1342781

I think it's better to go with #3988 instead. This achieves probably the same without too much dark magic. :)

LukasReschke · 2017-03-24T19:53:42Z

Replaced by #3988

ChristophWurst · 2017-03-24T19:58:38Z

Sure we saved some requests by combining js, but still FF loads stuff pretty slowly, which this PR actually addressed nicely. Can we re-open this and wait for FF dev's response? 🙏

benediktg · 2017-03-24T23:13:34Z

I agree – combining CSS and JS is rather a workaround for HTTP/1.1. And since every current version of nginx and Apache provides HTTP/2 support it's (IMHO) the Linux distributions' issue to support that and not Nextcloud's. ;)

Hence I would advise to not deny this PR.

ajs124 · 2017-03-24T23:40:29Z

Actually, I use HTTP/2 and firefox still takes half a minute to load the files app on my firefox instance, so that's not really a fix.

benediktg · 2017-03-24T23:50:35Z

@ajs124 Could also be a configuration or setup issue.

nsg · 2017-03-25T14:24:40Z

I should point out that HAProxy do not support HTTP/2 (maybe 6-12 months away, or more), with is a really popular proxy software to load balance if you have a larger site (or, special needs). It's is also likely that a larger cooperation will be stuck with proprietary load balances that only support HTTP/1 for years. So support for HTTP/1 is relevant still.

I did some dark magic to enable HTTP/2 and it was faster, but still, it takes me 15-20 seconds on an empty cache and that's not a good user experience. Nextcloud feels "heavy and slow", not the experience we like to expose to users. It makes it harder to sell the idea to replace proprietary solutions with Nextcloud if it feels really slow.

Still, I agree that this can't be merged if applications breaks, that is a worse user experience. If I understood this correctly most breakage is because libraries, like JQuery, are buggy with defer. I assume that will be fixed in the future.

In the future I think defer should be enabled (even if nice things like #3988 succeeds to limit the requests). Defer is a standard way to do things and it will speed up the user expertise for everyone. Maybe enable it by default in Nextcloud 13, 14 or 15, ...

Suggestion
Make this a optional feature, disabled by default. With an appropriate warning message that this is a experimental feature that may break applications. This has the advantage that application developers may be motivated to support defer.

Glandos · 2017-04-20T13:24:28Z

I applied this a few seconds ago, on my NextCloud 11.0.1 behind HTTP/2 server.
There's no way I revert it. It just works for me, and is incredibly fast. Thanks a lot.

I know, this is a useless comment, but I think congrats should be written sometimes.

michaelletzgus · 2017-04-20T14:22:54Z

I'm still using it on my productive system.
There seems to be slight problem with the current master version - but at a first glance this might be fixable relatively easy.

michaelletzgus · 2017-04-22T10:13:09Z

No problems in current master anymore. :-)

michaelletzgus · 2017-04-29T10:15:38Z

Update: Current master still working with almost all apps enabled. :)

Loading file app, 130 js requests (208 total):
9.9 sec vs. 19.3 sec

ViViDboarder · 2017-05-03T01:17:12Z

Even with concatenation, this would provide a speed improvement by allowing the browser to load the rest of the images and markup while downloading the single script file. Concatenation is definitely welcome, but deferring loading seems like a good thing to have.

If the worry is making things defer by default, what about adding a parameter to addScript that allows defer or async to be provided? This would allow even better optimization for things that one knows can be done done async and safer implementation of defer.

michaelletzgus · 2017-05-04T18:45:32Z

Still fully operational with current master and all apps except ownBackup - but ownBackup does not work either.
@ViViDboarder Good point, I'll try this...

ghost

Does anyone know if this applies to version 12? Or do these only apply to version 11 of Nextcloud server?

benediktg · 2017-08-29T16:13:58Z

@lawtechsupport See #4854, in Nextcloud 13 there is a patch included which enables deferred script loading. 😉

But you also can have it earlier:

wget https://patch-diff.githubusercontent.com/raw/nextcloud/server/pull/4854.patch -O/tmp/deferred-script-loading.patch
cd /your/nextcloud/document/root
patch -p1 < /tmp/deferred-script-loading.patch

This was referenced Mar 3, 2017

Deferred script loading #3656

Closed

NextCloud is unacceptably slow without asset pipelining #2272

Closed

nickvergessen reviewed Mar 6, 2017

View reviewed changes

ChristophWurst previously requested changes Mar 6, 2017

View reviewed changes

michaelletzgus force-pushed the deferred-script-loading-v2 branch 9 times, most recently from 1ec849c to e3a4e4c Compare March 8, 2017 08:18

michaelletzgus closed this Mar 8, 2017

michaelletzgus force-pushed the deferred-script-loading-v2 branch from e3a4e4c to b5a6f03 Compare March 8, 2017 08:19

michaelletzgus reopened this Mar 8, 2017

michaelletzgus force-pushed the deferred-script-loading-v2 branch 3 times, most recently from 02d0768 to e5fe6ab Compare March 9, 2017 17:12

sphrak mentioned this pull request Mar 9, 2017

Researching/Discussing web UI speed optimization/tweaks #3790

Closed

7 tasks

MorrisJobke added the 3. to review Waiting for reviews label Mar 9, 2017

michaelletzgus force-pushed the deferred-script-loading-v2 branch from 6d5b8cb to a39b996 Compare March 18, 2017 11:40

michaelletzgus mentioned this pull request Mar 18, 2017

Slow Nextcloud #3911

Closed

LukasReschke requested changes Mar 23, 2017

View reviewed changes

LukasReschke closed this Mar 24, 2017

michaelletzgus mentioned this pull request Mar 26, 2017

jQuery.globalEval()={} - Problem with password strength and files_markdown #4067

Closed

ghost reviewed Aug 29, 2017

View reviewed changes

Deferred script loading v2 #3696

Deferred script loading v2 #3696

Conversation

michaelletzgus commented Mar 3, 2017 • edited Loading

mention-bot commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristophWurst commented Mar 6, 2017

Choose a reason for hiding this comment

michaelletzgus Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benediktg commented Mar 7, 2017

MorrisJobke commented Mar 8, 2017

nickvergessen commented Mar 8, 2017

michaelletzgus commented Mar 8, 2017 • edited Loading

nickvergessen commented Mar 8, 2017

michaelletzgus commented Mar 8, 2017

MorrisJobke commented Mar 8, 2017

mm0 commented Mar 14, 2017

michaelletzgus commented Mar 14, 2017

michaelletzgus commented Mar 15, 2017

nickvergessen commented Mar 16, 2017

codecov-io commented Mar 18, 2017

Codecov Report

Bubu commented Mar 18, 2017

LukasReschke commented Mar 20, 2017

michaelletzgus commented Mar 20, 2017 • edited Loading

xdmx commented Mar 21, 2017

nickvergessen commented Mar 23, 2017

LukasReschke left a comment

Choose a reason for hiding this comment

LukasReschke commented Mar 24, 2017

ChristophWurst commented Mar 24, 2017

benediktg commented Mar 24, 2017

ajs124 commented Mar 24, 2017

benediktg commented Mar 24, 2017

nsg commented Mar 25, 2017 • edited Loading

Glandos commented Apr 20, 2017

michaelletzgus commented Apr 20, 2017

michaelletzgus commented Apr 22, 2017

michaelletzgus commented Apr 29, 2017 • edited Loading

ViViDboarder commented May 3, 2017

michaelletzgus commented May 4, 2017

ghost left a comment

Choose a reason for hiding this comment

benediktg commented Aug 29, 2017 • edited Loading

michaelletzgus commented Mar 3, 2017 •

edited

Loading

michaelletzgus Mar 6, 2017 •

edited

Loading

michaelletzgus commented Mar 8, 2017 •

edited

Loading

michaelletzgus commented Mar 20, 2017 •

edited

Loading

nsg commented Mar 25, 2017 •

edited

Loading

michaelletzgus commented Apr 29, 2017 •

edited

Loading

benediktg commented Aug 29, 2017 •

edited

Loading