Skip to content

Conversation

pulsejet
Copy link
Member

@pulsejet pulsejet commented Nov 25, 2022

Enables lazy loading of libraries in the JS bundle. This improves perceivable UI loading time by almost 2x (I realised my dev/test setup isn't ideal for testing this) in my test environment (to load the Files app) and reduces the transferred data from ~5MB to ~3MB. These results are shown beside the screenshots.

The most painful things loaded in core-common.js (now separate):

  1. Nextcloud vue components. These are huge and not always required.
  2. The password confirmation dialog. This one is the biggest.
  3. Emoji and moment JSON

Caveat:

  1. The total size of everything is slightly larger, but this is fine, since not everything is loaded (much less, actually)
  2. Slightly increased complexity. I doubt it's significant.

Production bundle before optimization:
before

After pruning:
after

import Moment from './Moment'
import CommentMixin from '../mixins/CommentMixin'
const NcRichContenteditable = () => import('@nextcloud/vue/dist/Components/NcRichContenteditable')
Copy link
Member

Choose a reason for hiding this comment

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

Is this something we could fix in our libraries?

Copy link
Member Author

Choose a reason for hiding this comment

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

Async components is a Vue feature. There's no bug, so there's nothing to "fix" as such.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah i more meant if we can help all users of our libraries to do that out of the box, instead of having to bring it manually to all apps and points

Copy link
Member Author

Choose a reason for hiding this comment

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

No. Whether a component can be lazy loaded depends on the assumptions the caller is making. For example, if some other function has some side effects that affects the component, lazy loading will break things. This is as out-of-the-box as it can get.

Copy link
Member Author

@pulsejet pulsejet Nov 25, 2022

Choose a reason for hiding this comment

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

One thing worth figuring out would be the reason @nextcloud/vue is so bloated (100KB per component at times). If that can be fixed then we don't need lazy loading at all. Another fix would be to stop bundling a lot of stuff in packages like @nextcloud/password-confirmation

Copy link
Member

Choose a reason for hiding this comment

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

The duplication that is in@nextcloud/vue is getting addressed in #36383

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const NcRichContenteditable = () => import('@nextcloud/vue/dist/Components/NcRichContenteditable')
const NcRichContenteditable = () => import(/* webpackPreload: true */ '@nextcloud/vue/dist/Components/NcRichContenteditable')

see https://webpack.js.org/guides/code-splitting/#prefetchingpreloading-modules

Signed-off-by: Varun Patil <varunpatil@ucla.edu>
@skjnldsv
Copy link
Member

skjnldsv commented Nov 26, 2022

Thanks for doing this and raising the topic @pulsejet

So, there is a fine line between performance and lazy loading.
While splitting into multiple apps package makes sense, i'm not super fond on the way it behaves.
Fetching and loading on a click action is making the UI looks veeeery slow and sluggish. One good example is the UserStatusModal

Have you tried clicking the Set status button in the top right menu? See how a few hundreds of ms is actually making it look very bad? 🙈

Loading an additional 1 or 2 MB on load, and considering it is cached, is actually not that bad. We have strong caching times.
There are surely some areas where it make sense, like loading it asynchronously (loading the main common bundle, then defer the loading of the UserStatusModal for example. But loading some components on-demand is something very bad I think :)

I haven't looked into details about your code yet, just putting this here for future discussions 😉

@skjnldsv
Copy link
Member

This improves perceivable UI loading time by almost 2x (I realised my dev/test setup isn't ideal for testing this) in my test environment (to load the Files app) and reduces the transferred data from ~5MB to ~3MB. These results are shown beside the screenshots.

Please also calculate how long it takes to transfer all of those files.
Fetching 20 tiny files is not always faster than one big one :)

@pulsejet
Copy link
Member Author

pulsejet commented Nov 26, 2022

@skjnldsv this is less about performance and more about correctness. Loading everything for all pages in a core-common.js is wrong since most apps don't need most of the loaded components. A few examples:

  1. A normal user will most almost never need the password confirmation dialog unless they're in settings. How often do you expect a user to change settings? The password-confirmation is ~20%(?!) of the bundle.
  2. Ditto for many other things. For example moment-timezone is only required in settings for setting some availability date.
  3. It gets even worse when talking about non-core apps. Consider Photos. It bundles it's own version of Nextcloud vue components and uses almost nothing from the whole bunch we just loaded in core-common.js.

Loading an additional 1 or 2 MB on load, and considering it is cached, is actually not that bad. We have strong caching times. There are surely some areas where it make sense, like loading it asynchronously (loading the main common bundle, then defer the loading of the UserStatusModal for example. But loading some components on-demand is something very bad I think :)

Yes, I agree. Ideally, we should have a few timers loading these scripts asynchronously a few seconds after the page loads. This way the page loads fast enough but there's no slugishness. Still I'd maintain that loading everything at the start to combat this isn't the right solution. My server, for instance, has user status disabled altogether, so I'm loading the ton of emoji on every page for really no reason.

I'm willing to look into this later if this sounds good (but only if this gets merged first).

Please also calculate how long it takes to transfer all of those files.
Fetching 20 tiny files is not always faster than one big one :)

It's irrelevant. As I mentioned above, most of these files never get fetched at all in "normal" usage. For loading the Files app, nothing extra needs to be fetched with this PR.

@pulsejet
Copy link
Member Author

pulsejet commented Nov 26, 2022

BTW another reason Nextcloud takes time to load in the browser is we load all apps scripts synchronously. For example, loading the viewer and sidebar scripts takes almost 200-300ms even on a fast machine. If we defer this to load a couple of seconds after the main view is loaded, the user is never going to know except for the better UX.

@pulsejet pulsejet removed the 2. developing Work in progress label Nov 27, 2022
@ChristophWurst
Copy link
Member

BTW another reason Nextcloud takes time to load in the browser is we load all apps scripts synchronously.

AFAIK the problem is that many scripts are still not standalone. They depend on some global variables, like OC and OCP. Therefore loading order and synchronous loading is what keeps those scripts from breaking.

There was an attempt to make apps less dependent on these globals through the @nextcloud/* packages. But I think we are still not at a point where we can load all scripts async. #30385 was added not long ago and reinforces the problem.

So I think the best we could do is add an option for apps to register their scripts as async-compatible and only load those asynchronously, at the end of the body. E.g. if the app Viewer works without OC/OCP/etc and no other script relies on the existence of global variables set by the Viewer app then the Viewer scripts could be loaded async. But making all scripts load async will likely cause problems, I'm afraid.

@pulsejet
Copy link
Member Author

Yeah, it isn't as easy as loading the scripts async. I doubt we can completely remove globals, but what I think would be ideal is load only the globals and critical parts synchronously and defer the rest to later (e.g. the Vue parts).

@skjnldsv
Copy link
Member

skjnldsv commented Nov 29, 2022

It's irrelevant. As I mentioned above, most of these files never get fetched at all in "normal" usage. For loading the Files app, nothing extra needs to be fetched with this PR.

Nextcloud is much more than the "files" app, my point still stand.

  • A normal user will most almost never need the password confirmation dialog unless they're in settings. How often do you expect a user to change settings? The password-confirmation is ~20%(?!) of the bundle.
  • Ditto for many other things. For example moment-timezone is only required in settings for setting some availability date.
  • It gets even worse when talking about non-core apps. Consider Photos. It bundles it's own version of Nextcloud vue components and uses almost nothing from the whole bunch we just loaded in core-common.js.

Fair points.

But the issue still stands. Even as an admin.
If the async loading happens automatically, then it's just deferred, and I'm ok with that.
If they happen on demand, I'm against it. It has too much of an impact on the UX

One good example is the UserStatusModal

@pulsejet
Copy link
Member Author

pulsejet commented Nov 29, 2022

Nextcloud is much more than the "files" app, my point still stand.

Okay then point me to a place where 20 files need to be loaded and I can measure it. I have not found any such place. The one place where the most number of chunks need loading (that I found) is the user status modal (6 chunks). Loading all of them without cache takes ~150ms for me including network latency (~80ms) and JS parsing and execution. HTTP pipelining negates a lot of latency cost.

Fair points.

But the issue still stands. If the async loading happens automatically, then it's just deferred, and I'm ok with that. If they happen on demand, I'm against it. It has too much of an impact on the UX

One good example is the UserStatusModal

I have three points to make:

  1. I measured this. With cache disabled, the user status modal consistently starts loading in <300ms for me. I couldn't feel the "too much impact" on UX, and I doubt most humans can. I don't a particularly fast machine either, and my dev server isn't local (it's on WAN).
  2. With cache, this drops to ~100ms (that's the approx parsing time, ~10ms for cache lookup). Now that's not perceivable at all since there's a much larger delay in the dialog opening animation (~400ms).
  3. So I don't know what you're trying to optimize here. There are two choices (it's always a trade off):
    1. Optimize assuming the normal use-case; everything will be cached most of the time, so we can lazy load components and it'll have negligible impact on user experience. On the up side, it will have imporved loading time on every page load.
    2. Load everything eagerly, so we better UX the first time the user opens some dialog the first time they open nextcloud on the device (or after Nextcloud is updated). And the down side is we have slower page loads for every page load. If you still think this makes sense then I cannot argue further; you can close this.

Even as an admin.

Do you mean it's fine to load admin-specific components for all users just so that the admin saves 200ms on some dialog (that too only on a cold cache)? I don't know what to say.

Regardless,

Ideally, we should have a few timers loading these scripts asynchronously a few seconds after the page loads. This way the page loads fast enough but there's no slugishness. I'm willing to look into this later if this sounds good (but only if this gets merged first).

Of course I have to mention this has risks of it's own. If the user is already interacting with the page (read scrolling) while we load the scripts, it has the potential to freeze up the UI creating even worse UX. When the user clicks on a button and there's a slight delay that's better because at least they might expect it.

@skjnldsv
Copy link
Member

Do you mean it's fine to load admin-specific components for all users just so that the admin saves 200ms on some dialog (that too only on a cold cache)? I don't know what to say.

Obviously not 😁
But that admin should also be taken into consideration in the thinking process :)

@skjnldsv
Copy link
Member

I measured this. With cache disabled, the user status modal consistently starts loading in <300ms for me. I couldn't feel the "too much impact" on UX, and I doubt most humans can. I don't a particularly fast machine either, and my dev server isn't local (it's on WAN).

Okay, so. I'm on your side, I'm the one that actually added the async loading in apps/users management and I'm also the one that added the UserStatusModal...
The discussion we're having is not new and i've had it like 4-5 times at Nextcloud over the last few years 😅

I'm saying this because you can absolutely feel the sluggishness of the nextcloud load, even for tiny chunks.
There are tons of studies [1] on it, users will only feel something is instant on anything under 100ms. I just loaded the modal by clicking the "set status" button and it took around 150ms, and I can tell you I felt it already. And i'm not even talking about slow 3G or DSL types of phones yet. Only server time-to-respond :)

I don't have time to invest into this area for now.
I've raised my concerns, not saying this is a bad direction. But it comes with fallbacks and can have unwanted outcomes. I'm gonna leave others reviews and approve/merge if they want 🤗
Most of it is probably a good idea nonetheless 👍 .

@pulsejet
Copy link
Member Author

Understood. As I mentioned, it's a trade off to think about; I'll wait for reviews from others. Would be nice if more people can actually test this.

I'm saying this because you can absolutely feel the sluggishness of the nextcloud load

That's exactly my problem, just in a different place haha. The initial page load is too slow for me, and that happens every time I open nextcloud or switch to a different app.

@skjnldsv skjnldsv removed their request for review November 30, 2022 07:39
@pulsejet
Copy link
Member Author

pulsejet commented Dec 7, 2022

Nobody wants to review this?

import { generateOcsUrl } from '@nextcloud/router'
import { confirmPassword } from '@nextcloud/password-confirmation'
import '@nextcloud/password-confirmation/dist/style.css'
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword()
const confirmPassword = async () => await (await import(/* webpackPreload: true */ '@nextcloud/password-confirmation')).confirmPassword()

import axios from '@nextcloud/axios'
import { confirmPassword } from '@nextcloud/password-confirmation'
import '@nextcloud/password-confirmation/dist/style.css'
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword()
const confirmPassword = async () => await (await import(/* webpackPreload: true */ '@nextcloud/password-confirmation')).confirmPassword()

import { generateOcsUrl } from '@nextcloud/router'
import { confirmPassword } from '@nextcloud/password-confirmation'
import '@nextcloud/password-confirmation/dist/style.css'
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword()
const confirmPassword = async () => await (await import(/* webpackPreload: true */ '@nextcloud/password-confirmation')).confirmPassword()

import debounce from 'debounce'
import NcColorPicker from '@nextcloud/vue/dist/Components/NcColorPicker'
import Tooltip from '@nextcloud/vue/dist/Directives/Tooltip'
const NcColorPicker = () => import('@nextcloud/vue/dist/Components/NcColorPicker')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const NcColorPicker = () => import('@nextcloud/vue/dist/Components/NcColorPicker')
const NcColorPicker = () => import(/* webpackPreload: true */ '@nextcloud/vue/dist/Components/NcColorPicker')

import { confirmPassword } from '@nextcloud/password-confirmation'
import '@nextcloud/password-confirmation/dist/style.css'
import { print } from '../service/PrintService'
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword()
const confirmPassword = async () => await (await import(/* webpackPreload: true */ '@nextcloud/password-confirmation')).confirmPassword()

<script>
import NcButton from '@nextcloud/vue/dist/Components/NcButton.js'
import NcEmojiPicker from '@nextcloud/vue/dist/Components/NcEmojiPicker.js'
const NcEmojiPicker = () => import('@nextcloud/vue/dist/Components/NcEmojiPicker.js')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const NcEmojiPicker = () => import('@nextcloud/vue/dist/Components/NcEmojiPicker.js')
const NcEmojiPicker = () => import(/* webpackPreload: true */ '@nextcloud/vue/dist/Components/NcEmojiPicker.js')

import NcEmptyContent from '@nextcloud/vue/dist/Components/NcEmptyContent'
import moment from '@nextcloud/moment'
const DashboardWidget = () => import('@nextcloud/vue-dashboard')
const DashboardWidgetItem = () => import('@nextcloud/vue-dashboard')
Copy link
Member

Choose a reason for hiding this comment

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

This seems broken


<script>
import MultiselectTags from '@nextcloud/vue/dist/Components/NcMultiselectTags.js'
const MultiselectTags = () => import('@nextcloud/vue/dist/Components/NcMultiselectTags.js')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const MultiselectTags = () => import('@nextcloud/vue/dist/Components/NcMultiselectTags.js')
const MultiselectTags = () => import(/* webpackPreload: true */ '@nextcloud/vue/dist/Components/NcMultiselectTags.js')

import RequestTime from './RequestTime'
import RequestURL from './RequestURL'
import RequestUserGroup from './RequestUserGroup'
const RequestUserAgent = () => import('./RequestUserAgent')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const RequestUserAgent = () => import('./RequestUserAgent')
const RequestUserAgent = () => import(/* webpackPreload: true */ './RequestUserAgent')

import RequestURL from './RequestURL'
import RequestUserGroup from './RequestUserGroup'
const RequestUserAgent = () => import('./RequestUserAgent')
const RequestTime = () => import('./RequestTime')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const RequestTime = () => import('./RequestTime')
const RequestTime = () => import(/* webpackPreload: true */ './RequestTime')

import RequestUserGroup from './RequestUserGroup'
const RequestUserAgent = () => import('./RequestUserAgent')
const RequestTime = () => import('./RequestTime')
const RequestURL = () => import('./RequestURL')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const RequestURL = () => import('./RequestURL')
const RequestURL = () => import(/* webpackPreload: true */ './RequestURL')

const RequestUserAgent = () => import('./RequestUserAgent')
const RequestTime = () => import('./RequestTime')
const RequestURL = () => import('./RequestURL')
const RequestUserGroup = () => import('./RequestUserGroup')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const RequestUserGroup = () => import('./RequestUserGroup')
const RequestUserGroup = () => import(/* webpackPreload: true */ './RequestUserGroup')

import { confirmPassword } from '@nextcloud/password-confirmation'
import '@nextcloud/password-confirmation/dist/style.css'
import { loadState } from '@nextcloud/initial-state'
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const confirmPassword = async () => await (await import('@nextcloud/password-confirmation')).confirmPassword()
const confirmPassword = async () => await (await import(/* webpackPreload: true */ '@nextcloud/password-confirmation')).confirmPassword()

@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
This was referenced May 3, 2023
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels May 9, 2023
@skjnldsv skjnldsv modified the milestones: Nextcloud 27, Nextcloud 28 May 9, 2023
@pulsejet pulsejet removed this from the Nextcloud 28 milestone May 17, 2023
@pulsejet
Copy link
Member Author

Superceded by #38329

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants