Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up fetching of WebDAV folder listing #31160

Closed
wants to merge 2 commits into from

Conversation

CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Feb 13, 2022

Before this patch, the WebDAV requests were done once all the scripts are
executed and the DOMContentLoaded events is triggered. This is an issue
because it takes a long time for all the scripts to be run (~900ms).

Furthermore, once the request is done the CPU is idle because there is no
script anymore to fetch and it's just waiting for the response.

This patch makes the script do the WebDAV when executing the script
instead of when DOMContentLoaded is executed. This has the advantage
that during the WebDAV requests, the browser is not idle but continue
executing the other scripts (thanks to the async IO).

End result: the file listing is displayed ~100-200ms earlier

This also makes sure that the files_sharing scripts are executed earlier
than the files scripts as they have event listeners on the files apps.

Chromium profiler result before this change:

image

Chromium profiler results after this change:

image

Todos:

  • Fix apps that expected the files app to not be already be fully loaded (e.g. text app, but probably more)

Before this patch, the WebDAV requests was done once all the scripts are
executed and that the DOMContentLoaded is executed. This is an issue
because it takes a long time for all the scripts to be run (~900ms).

Futhermore, once the request is done the CPU is idle because there is no
script anymore to fetch and it's just waiting for the response.

This patch makes the script do the WebDAV when executing the script
instead of when DOMContentLoaded is executed. This has the advantage
that during the WebDAV requests, the browser is not idle but continue
executing the other scripts (thanks to the async IO).

End result: the file listing is displayed 100ms earlier

This also makes sure that the files_sharing scripts are executed earlier
than the files scripts as they have event listeners on the files apps.

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan CarlSchwan added the 2. developing Work in progress label Feb 13, 2022
@CarlSchwan CarlSchwan added this to the Nextcloud 24 milestone Feb 13, 2022
@CarlSchwan CarlSchwan self-assigned this Feb 13, 2022
@CarlSchwan
Copy link
Member Author

This will need a bit more thinking. We need to execute OC.Plugins.attach('OCA.Files.*) after DOMContentLoaded is triggered to load the other apps correctly.

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@juliusknorr
Copy link
Member

Haven't tested yet, but this might have potential to break apps that register a plugin on the file list to extend the properties fetched during propfind (e.g. groupfolders/files_lock) if the registration then happens too late.

https://github.com/search?q=org%3Anextcloud+addFileInfoParser&type=code would be a list of things we should check at least.

@CarlSchwan
Copy link
Member Author

I didn't test other apps but at least the groupfolders ACL in the sidebar still works

@juliusknorr
Copy link
Member

Tested a bunch and they indeed do not seem to be working anymore:

  • files_sharing additional property for showing the share owner
  • comments property for displaying unread comment mentions
  • group folders (while acls in the sidebar works, after setting one and a hard reload the existing ACL rules which are read from the propfind are not fetched)
  • files_lock

Mainly the hot-patching of the files client that is done in those apps does not happen anymore before the file list issues the propfind on the initial load, so this works again on the second propfind, just the first is broken :/

@CarlSchwan
Copy link
Member Author

Tested a bunch and they indeed do not seem to be working anymore:

* [ ]  files_sharing additional property for showing the share owner

* [ ]  comments property for displaying unread comment mentions

* [ ]  group folders (while acls in the sidebar works, after setting one and a hard reload the existing ACL rules which are read from the propfind are not fetched)

* [ ]  files_lock

Mainly the hot-patching of the files client that is done in those apps does not happen anymore before the file list issues the propfind on the initial load, so this works again on the second propfind, just the first is broken :/

I suppose this will need to define in php, that parameter will need to be fetched if I understand correctly. Unfortunately this means adding new api (topic: file info api) and something not backportable

@@ -398,10 +393,4 @@
};
})();

window.addEventListener('DOMContentLoaded', function() {
// wait for other apps/extensions to register their event handlers and file actions
Copy link
Member

Choose a reason for hiding this comment

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

seems you discarded this logic
now, this was there for apps that still register their file actions using the global FileActions object instead of the namespaces one. Not sure how many such apps still exist nowadays.

@PVince81
Copy link
Member

if we go this way I'd advise to not backport because this could break apps that are using the old approach for registering file actions.

as far as I remember there was a potential loading order issue that if an app's JS code is loaded and running before the files app is initialized, then it wouldn't have access to the global FileActions object, and possibly also not the OCA.Files namespace.

it all depends how apps are written.

might be good to retest with a few apps that register file actions and those that hook into the files app also

@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
@blizzz blizzz mentioned this pull request Mar 31, 2022
@blizzz blizzz mentioned this pull request Apr 7, 2022
@CarlSchwan CarlSchwan modified the milestones: Nextcloud 24, Nextcloud 25 Apr 7, 2022
@PVince81
Copy link
Member

ideal would be to have the file list array separated from the FileList view class
this way we could already trigger fetching much earlier

this is what we'd do anyway once porting the file list to Vue

This was referenced Aug 12, 2022
This was referenced Aug 24, 2022
This was referenced Sep 6, 2022
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
This was referenced Sep 20, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@CarlSchwan CarlSchwan mentioned this pull request Oct 26, 2022
@blizzz blizzz mentioned this pull request Feb 1, 2023
@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
@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@skjnldsv
Copy link
Member

Outdated with new Files app

@skjnldsv skjnldsv closed this Feb 27, 2024
@skjnldsv skjnldsv deleted the decrease-latency-file branch March 14, 2024 07:44
@skjnldsv skjnldsv removed this from the Nextcloud 29 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants