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

Reduce number of database queries during WebDAV propfind request #34471

Merged
merged 2 commits into from
Oct 14, 2022

Conversation

starypatyk
Copy link
Contributor

While working on nextcloud/android#10783 I have found the following.

  • When reading complete data for a folder the Android client requests the nc:note attribute in the WebDAV propfind request.
  • Current implementation of retrieval of this attribute executes four separate database queries for each file in the folder.

With help of @PVince81 I have been able to refactor the code to avoid issuing additional database queries, since required data is already loaded into memory.

Performance is improved significantly - almost four times faster in my tests (331 files in a folder)

_ Execution time [ms] # of DB queries
Before 1300 1699
After 350 372

Proposed changes introduce a slight change in behaviour when a file has been shared more than once with the same user - e.g. with a direct share and a group share.

  • Previously in this case only note from the first share found was returned.
  • Now notes from all shares are concatenated (with a new-line) and returned.

With the new implementation the notes are presented like this in the Android client:

I have not been able to find the share notes in the UI of the web client.

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>
@starypatyk
Copy link
Contributor Author

I have looked at the failing tests.

Apparently the performance-8.0 test is not prepared for PRs from cloned repositories.

Run git fetch origin dav_read_share_notes
fatal: couldn't find remote ref dav_read_share_notes
Error: Process completed with exit code 128.

Regarding acceptance tests - the errors are suspiciously close to my changes (both relate to the sharing feature). Unfortunately I have not been able to correlate the reported errors with potential errors in the backend. Are logs from the backend available somewhere for the failed run?

@szaimen szaimen added this to the Nextcloud 26 milestone Oct 9, 2022
@PVince81
Copy link
Member

@starypatyk the acceptance test issues are not related to your code changes. These are tests that randomly fail on CI (but not locally) which we were not able to fix yet.

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 see minor comment

apps/dav/lib/Connector/Sabre/Node.php Outdated Show resolved Hide resolved
@PVince81
Copy link
Member

/backport to stable25

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>
@PVince81 PVince81 added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 11, 2022
@PVince81
Copy link
Member

I've restarted the CI jobs that timed out.

Then this should be good to merge

@starypatyk
Copy link
Contributor Author

I am not sure about next steps. The tests are successful - except performance tests (irony 😉) that seem not to be prepared when PR comes from a fork.

Should I merge it myself? I am not even sure that I have required permissions...
Or should I wait for merge by someone from the server team?

I have no problem with the delay, but I would not like to block things.

@PVince81 PVince81 merged commit 95d45c2 into nextcloud:master Oct 14, 2022
@PVince81
Copy link
Member

@starypatyk thanks a lot for your contribution!

we'll take it from there, it will get backported to stable25 and after 25.0.0 is out we can merge the backport for 25.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish client: 🤖🍏 mobile feature: dav performance 🚀
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants