X Tutup
Skip to content

avoid retrieving user root iteratively in share controller#38055

Merged
phil-davis merged 1 commit intomasterfrom
bugfix/sharefilelist-perf-problem
Nov 12, 2020
Merged

avoid retrieving user root iteratively in share controller#38055
phil-davis merged 1 commit intomasterfrom
bugfix/sharefilelist-perf-problem

Conversation

@mrow4a
Copy link
Contributor

@mrow4a mrow4a commented Oct 29, 2020 •
edited
Loading

Fixes performance problem caused by code that was not refactored for long time.

Prerequisite:

  • Create 100 user shares from test1 to test2 root folder
  • Listing folders mounted for user test2-> 2s (complexity O(m), m->complexity of propfind, m is small overhead)

Attempt to reduce complexity but reusing available properties from "propfind kind of operation":

  • Opening shared with you panel before fix -> 15s complexity O(m+n), n>>m -> O(n)
  • Opening shared with you panel after attempt to fix, but due to need to return properties of mount like storage, storage_id it is not possible -> 2s complexity O(m)
  • Opening shared with you panel after a fix that just reduced impact of n -> 7s complexity O(m+n), n>m -> O(n)

Finally, I settled on being required on reducing to 7s from 15s, but still not perfect as we need to respond with many mount properties for /shares .. paging would solve the problem best.

Zrzut ekranu 2020-10-29 o 21 41 11

@mrow4a mrow4a requested a review from jvillafanez October 29, 2020 20:44
@mrow4a mrow4a self-assigned this Oct 29, 2020
@update-docs
Copy link

update-docs bot commented Oct 29, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@mrow4a mrow4a changed the title avoid retrieving user root and share nodes iteratively avoid retrieving user root and share nodes iteratively in share controller Oct 29, 2020
@mrow4a mrow4a marked this pull request as draft October 29, 2020 20:49
@phil-davis
Copy link
Contributor

phil-davis commented Oct 30, 2020

"something happened" - there are unit test fails, and acceptance test fails, for lots of sharing scenarios.

@mrow4a
Copy link
Contributor Author

mrow4a commented Oct 30, 2020 •
edited
Loading

@phil-davis seems pending shares broke, I suspected this might happen (thus draft). Unit tests failures are expected due to logic change (e.g. that function is called 1 times instead of 10 times, but lets see..)

This PR is legacy code fight, so might need strong QA after.

@phil-davis
Copy link
Contributor

phil-davis commented Oct 30, 2020

This PR is legacy code fight, so might need strong QA after.

CI has a good range of sharing and resharing test scenarios. So if you get CI green, it will be 99% good-to-go - we will just need to look at the code changes and imagine if there are weird corner-cases to double-check.

mrow4a
mrow4a commented Oct 30, 2020
View reviewed changes
@mrow4a mrow4a force-pushed the bugfix/sharefilelist-perf-problem branch 2 times, most recently from 1bd0c00 to 1e0f1ac Compare November 1, 2020 10:21
@mrow4a mrow4a marked this pull request as ready for review November 1, 2020 12:01
@mrow4a
Copy link
Contributor Author

mrow4a commented Nov 1, 2020

@jvillafanez can you have a look? I tried to equalize the performance to PROPFIND, but it is not possible as OCS requires to return a set of fields that makes it not possible.

Proper solution would be paging..

jvillafanez
jvillafanez reviewed Nov 3, 2020
View reviewed changes
@mrow4a mrow4a force-pushed the bugfix/sharefilelist-perf-problem branch from 1e0f1ac to 49f7150 Compare November 11, 2020 20:15
@mrow4a mrow4a requested a review from jvillafanez November 11, 2020 20:24
@mrow4a mrow4a changed the title avoid retrieving user root and share nodes iteratively in share controller avoid retrieving user root iteratively in share controller Nov 11, 2020
@mrow4a mrow4a force-pushed the bugfix/sharefilelist-perf-problem branch from 49f7150 to b542656 Compare November 11, 2020 21:51
phil-davis
phil-davis reviewed Nov 12, 2020
View reviewed changes
jvillafanez
jvillafanez approved these changes Nov 12, 2020
View reviewed changes
Copy link
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

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

Code looks fine. Need to fix typos

@mrow4a
Copy link
Contributor Author

mrow4a commented Nov 12, 2020

please squash the PR when merging

@phil-davis
Copy link
Contributor

phil-davis commented Nov 12, 2020

I will squash now...

@phil-davis phil-davis force-pushed the bugfix/sharefilelist-perf-problem branch from afbbfd1 to d42900f Compare November 12, 2020 11:55
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 12, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@phil-davis phil-davis merged commit be5906f into master Nov 12, 2020
@delete-merged-branch delete-merged-branch bot deleted the bugfix/sharefilelist-perf-problem branch November 12, 2020 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jvillafanez jvillafanez jvillafanez approved these changes

@phil-davis phil-davis phil-davis approved these changes

Assignees

@mrow4a mrow4a

Labels

4 - To release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mrow4a @phil-davis @jvillafanez
X Tutup