Skip to content

Conversation

@michaelletzgus
Copy link
Contributor

Deferred script loading can not be turnes on/off via config.php:

'deferred_script_loading' => true,

@mention-bot
Copy link

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

@michaelletzgus michaelletzgus force-pushed the deferred-script-loading-v2 branch 2 times, most recently from 9a197ab to 90d89e4 Compare May 13, 2017 14:06
@michaelletzgus
Copy link
Contributor Author

Default page load (file manager): 23 sec
With deferred enabled: 10 sec

Enabled Apps:

  - admin_audit: 1.2.0
  - admin_notifications: 1.0.0
  - bruteforcesettings: 1.0.2
  - calendar: 1.5.2
  - checksum: 0.3.4
  - circles: 0.9.6
  - comments: 1.2.0
  - contacts: 1.5.3
  - dav: 1.3.0
  - deck: 0.1.4
  - direct_menu: 0.10.0
  - external: 2.0.1
  - federatedfilesharing: 1.2.0
  - federation: 1.2.0
  - files: 1.7.2
  - files_automatedtagging: 1.2.2
  - files_downloadactivity: 1.1.1
  - files_external: 1.3.0
  - files_external_sia: 0.1.2
  - files_markdown: 1.0.1
  - files_opds: 0.8.2
  - files_reader: 1.0.4
  - files_retention: 1.1.2
  - files_sharing: 1.4.0
  - files_trashbin: 1.2.0
  - files_versions: 1.5.0
  - githubmergetracker: 0.0.17
  - gpxedit: 0.0.5
  - gpxpod: 2.1.1
  - groupfolders: 1.0.2
  - impersonate: 1.0.1
  - issuetemplate: 0.2.1
  - lookup_server_connector: 1.0.0
  - mail: 0.6.4
  - news: 11.0.0
  - nextant: 1.0.8
  - notes: 2.2.0
  - ocsms: 1.12.0
  - onlyoffice: 1.0.4
  - ownbackup: 17.5.0
  - passman: 2.1.2
  - previewgenerator: 1.0.6
  - provisioning_api: 1.2.0
  - qownnotesapi: 17.5.0
  - rainloop: 4.28.1
  - registration: 0.2.3
  - sharebymail: 1.2.0
  - sharepoint: 1.0.2
  - socialsharing_diaspora: 1.0.1
  - socialsharing_email: 1.0.1
  - socialsharing_facebook: 1.0.1
  - socialsharing_googleplus: 1.0.1
  - socialsharing_twitter: 1.0.1
  - spreed: 2.0.0
  - spreedme: 0.3.8
  - systemtags: 1.2.0
  - tasks: 0.9.5
  - testing: 1.2.0
  - theming: 1.3.0
  - twofactor_backupcodes: 1.1.0
  - twofactor_totp: 1.3.0
  - twofactor_u2f: 1.3.1
  - updatenotification: 1.2.0
  - weather: 1.4.1
  - workflowengine: 1.2.0

@codecov
Copy link

codecov bot commented May 13, 2017

Codecov Report

Merging #4854 into master will increase coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #4854      +/-   ##
============================================
+ Coverage     54.16%   54.16%   +<.01%     
- Complexity    22276    22278       +2     
============================================
  Files          1379     1379              
  Lines         85301    85308       +7     
  Branches       1322     1322              
============================================
+ Hits          46203    46208       +5     
- Misses        39098    39100       +2
Impacted Files Coverage Δ Complexity Δ
core/templates/layout.user.php 0% <ø> (ø) 0 <0> (ø) ⬇️
core/templates/layout.base.php 0% <ø> (ø) 0 <0> (ø) ⬇️
core/templates/layout.guest.php 0% <ø> (ø) 0 <0> (ø) ⬇️
lib/private/legacy/template/functions.php 10.63% <0%> (+3.74%) 0 <0> (ø) ⬇️
lib/private/legacy/template.php 30% <0%> (-0.41%) 40 <0> (+2)
lib/private/Files/Cache/Propagator.php 94.93% <0%> (-1.27%) 16% <0%> (ø)
apps/theming/lib/ThemingDefaults.php 87.27% <0%> (-0.23%) 35% <0%> (ø)
lib/private/Server.php 93.27% <0%> (-0.15%) 120% <0%> (ø)
core/js/js.js 61.78% <0%> (+0.56%) 0% <0%> (ø) ⬇️
lib/private/Security/CertificateManager.php 91.83% <0%> (+1.02%) 39% <0%> (ø) ⬇️
... and 2 more

@michaelletzgus michaelletzgus force-pushed the deferred-script-loading-v2 branch 3 times, most recently from 03ba956 to 4424990 Compare May 14, 2017 07:35
@MorrisJobke
Copy link
Member

Deferred script loading can not be turnes on/off via config.php:

'deferred_script_loading' => true,

I really don't like such kind of things. If we think this is the way to go: we should do it. If not: then not. But we should definitely not offload this to administrators. That is a bad excuse for developers about being not sure about the outcome of a change and being able to blame the admin then if something breaks. :/

I would go for adding this by default right after we branched of stable12 and are heading to the 13 release.

@MorrisJobke MorrisJobke added this to the Nextcloud 13 milestone May 14, 2017
@ChristophWurst
Copy link
Member

I would go for adding this by default right after we branched of stable12 and are heading to the 13 release.

Makes a lot of sense.

Deferred loading makes a huge difference on FF page loaded, even cached assets are loaded faster it seems and I don't see blank pages when switching apps – which is pretty cool :)

@rullzer
Copy link
Member

rullzer commented May 16, 2017

Agreed. Lets do it early for 13!

@michaelletzgus michaelletzgus force-pushed the deferred-script-loading-v2 branch from 4424990 to 698bca2 Compare May 16, 2017 16:19
@michaelletzgus
Copy link
Contributor Author

It's now enabled by default - but can still be disabled with "disable_deferred_script_loading" => true.
The disabling mechanism is easy to remove, if desired...

@michaelletzgus michaelletzgus force-pushed the deferred-script-loading-v2 branch from 698bca2 to b2519c7 Compare May 16, 2017 16:25
@MorrisJobke
Copy link
Member

The disabling mechanism is easy to remove, if desired...

Kill it. Less options means less code that can break and less permutations of possible configurations.

@MorrisJobke
Copy link
Member

stable12 is branched of. So I'm now fine with this, once my comment is addressed :)

@michaelletzgus michaelletzgus force-pushed the deferred-script-loading-v2 branch from b2519c7 to ba10d7b Compare May 19, 2017 16:07
@michaelletzgus
Copy link
Contributor Author

Done!
No config option any more.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works here 👍 (even in IE and Edge ;))

@MorrisJobke
Copy link
Member

Ignore CI - the tests failures are unrelated.

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 19, 2017
@MorrisJobke MorrisJobke requested a review from juliusknorr May 19, 2017 17:44
* Create generalized function for emmitting <script defer src=""> tags to templates
* Remove type attribute from inline_js
* Add defer attribute to external <script> tags

Signed-off-by: Michael Letzgus <michaelletzgus@users.noreply.github.com>
@michaelletzgus michaelletzgus force-pushed the deferred-script-loading-v2 branch from ba10d7b to fb9f13d Compare May 20, 2017 11:44
@MorrisJobke MorrisJobke merged commit 0dae494 into nextcloud:master May 20, 2017
@michaelletzgus michaelletzgus deleted the deferred-script-loading-v2 branch May 21, 2017 09:08
@andrewborell
Copy link

andrewborell commented Nov 24, 2017

Didn't dig into this very far yet but I tested this patch in my NC v12.0.3 instance where it seemed to break OnlyOffice integration. I notice NC v13 milestone on this patch and I have not tested against that version to confirm if the issue persists.

Browser debug console indicates a "reference error: OCA is not defined" ( across all browsers, including mobile). No error is logged in nextcloud.log. Removed with patch -p1 -R < 4854.patch -- error went away immediately and integration resumed working as expected.

That issue aside I saw a very big improvement in the load times. Let me know if you need more info to reproduce integration issue.

@ChristophWurst
Copy link
Member

Please open a new issue. Thanks.

@andrewborell
Copy link

andrewborell commented Dec 6, 2017

@ChristophWurst After a little debugging I came to find 100% of my performance issue ( and evidently a javascript issue for jsxc.xmpp... CAPS error and jsxc.xmpp failed to load settings error ) were all resolved by configuring the site to use php-fpm. Hard to believe I overlooked that configuration. Speaking in terms of the compatibility of this fix with NCv12/13 as I posted in my previous message, I havent had a moment to go back and test things out since configuring fpm for the site.

I went back through the NCv12 guide and searched for FPM\FCGI to check best practice and all I really found was this:

mod_spdy together with libapache2-mod-php5 / mod_php (use fcgi or php-fpm instead)

I cannot speak on behalf of nginx however under apache2 I find this software only performs well using fpm. Testing with a single user connected you would never notice a difference, but when you get 15 or 20 users connected it's a difference of minutes for TTFB just to bring up the login page.

eta, that on Ubuntu16.04 / Apache 2.4.29 the FcgidMaxProcesses setting in the apache directives was causinig apache2 to fail startup, so there may have been another underlying issue that I'm not willing to investigate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants