-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
MountManager::find called too often for the same entries #26702
Comments
I've extracted the paths from the entries, so it looks like this:
|
This will break the logic. The purpose of the loop is to find the longest match. |
Now wondering if that "longest match" logic even makes sense here... |
It was also said in the past that |
A possible alternative implementation for longest match would be to have all mount points stored in an array as keys. Then iterate over the path sections of $path. So if
|
The number of iterations would then depend on the number of path subsections instead of the number of registered mounts. Should we try this out ? (I hope there are unit tests to confirm that it still works...) |
Hope this will be usefull: Average results from 3 tests -> owncloud/smashbox#152. This test will create, sync & share n files in the root directory. For each single file, it will execute sync(PROPFIND&PUT) and after that, share. These are results for oc82 and oc90, I enabled the same apps there: |
For oc90, both sync and share raises faster, look 1st and last requests values on y axis |
9.0 shows increasing share request duration with n. How are the Sync / Propfind times of the share receiver with n? |
In this test, share receiver just checks consistensy, he just receives files at the end. I can increase N to 1000 or 10000 if you like, it is automatised test |
Receivers Propfind performance would be very interesting |
Ok, now I get the problem. I will insert statistic both for sender and receiver of the share, for N create&sync&share. After the exam tomorrow, because will need to modify the script a little to log more things, so that I can "grep" it later. |
Any feedback on my proposed approach #26702 (comment) ? |
Hmm, Maybe a B-Tree of paths? It will be a little bit of code, but fast :) |
But if this is only to be used once, I think array might work. |
Ok, I'll have a try with the linear approach then and let you test it again. |
I have other task plus univ now, go for it. |
Will do, thanks |
Can you also sort it? This might amortize the search criticaly, when you check next string to be conmpletely different, it means you have longest match. But still, not sure if you can have it sorted while creating. |
Hmm, I thought these were already sorted alphabetically. That's why there's code to reverse the array to have a reverse sort. Will have to recheck. |
But do you get the idea of sorting? If you sort, you at max have to to that operation n times as it is now, and at minimum 1. Because if you have a match, you save it, check next, OHH, completely different, have longest match. |
regarding #26702 (comment): But maybe I am not familiar enough with the fs code ... |
Ah, the sorting I was thinking about is elsewhere. I suppose there might also be a bottleneck there: https://github.com/owncloud/core/blob/v9.0.4/lib/private/files/view.php#L1661 ( |
Yes, this is a local table in memory. If one day we have the in-DB FSTAB implementation then either the search will be done directly on the DB or in a local cached table like here after loading it into memory. |
PR here: #26813 |
Yes, please. Already asked in the PR. |
Unfortunetely, PR does not change anything from smashbox perspective - Patch did not change anything visible actualy, will check with blackfire. |
@mrow4a can you double check that the patch was actually applied ? If yes, one additional thing we could try is caching the search results after we found them. |
In the next moment I will blackfire sharing generaly. The amount of calculations in memory will not matter unless they perform a query to the database. It doesnt seem to be case here. I will research the sharing topic further, finding where the query is being issued and how many times. |
Is that so ? If a function is called a million times for nothing outside the DB, it is also likely that it will waste a lot of time. Let's see what your blackfire result says anyway. |
Surely it matters. I have just been talking that it will not matter in 10-100 shares scale so it will be that visible, refering to previous post with the smashbox test. |
Summary of investigation:
That PR is not backportable but I'll see if some smaller pieces of it could already improve performance of 9.0 to be like 9.1. Not sure if this is in any way related to MountManager::find though. |
PR is ready for testing: #26912 |
PR is merged |
I'm closing this for now. Please reopen if it still happens with 9.0.8. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Steps
for I in $(seq 1 50); do curl -u admin:admin -X MKCOL "http://localhost/owncloud/remote.php/webdav/test$I"; done
for I in $(seq 1 50); do curl -u admin:admin -X POST "http://localhost/owncloud/ocs/v1.php/apps/files_sharing/api/v1/shares?format=json" --data "shareType=user&shareWith=user1&path=test$I"; done
Mount\Manager::find()
to log the pathcurl -D - -u user1:test -X PUT http://localhost/owncloud/remote.php/webdav/bacon.txt
Expected result
Call to MountManager::find only once per file/folder
Actual result
Repeated calls.
Versions
Observed on 9.0.6
This means we might need to optimize and/or cache some of the results.
Or find out why it's called that often...
My results:
@butonic
The text was updated successfully, but these errors were encountered: