do not iterate over all storages when you only need a single node anyway #33302
Conversation
| } | ||
|
|
||
| return $nodes[0]; | ||
| return \array_shift($nodes); |
There was a problem hiding this comment.
Why? calling a built in function is more expensive then staying in the language
There was a problem hiding this comment.
indeed, this would not only return the first element but also need to update the array
There was a problem hiding this comment.
ack .... wanted to unify this and several places used this. will change to array access
| * returns only the first node because we only really need the id anyway | ||
| */ | ||
| private function getNode() { | ||
| $folder = \OC::$server->getUserFolder(); |
There was a problem hiding this comment.
please inject if possible - yes - my view should have been injected as well ....
| * @return resource | ||
| * @since 10.0.9 | ||
| */ | ||
| public function getContentOfVersion($internalPath, $versionId) { |
There was a problem hiding this comment.
funny ..... was that missed somehow?
|
@butonic drone is red - please have a look as well - THX |
butonic
force-pushed
the
fix-versions-performance
branch
from
a947244 to
88d9dff
Compare
Codecov Report
@@ Coverage Diff @@
## master #33302 +/- ##
============================================
+ Coverage 64.42% 64.76% +0.34%
- Complexity 18320 18330 +10
============================================
Files 1197 1198 +1
Lines 69264 69382 +118
Branches 1276 1276
============================================
+ Hits 44625 44938 +313
+ Misses 24267 24072 -195
Partials 372 372
Continue to review full report at Codecov.
|
|
I used |
|
I wanted to inject the UserFolder ... but .. besides that requiring a lot of changes ... urgh ... the meta endpoint is hopelessly inefficient. For every request to the versions endpoint As a result the same file id is looked up twice for each version AND twice for every thumbnail. But there is no caching whatsoever ... This calls for cacheception: #28166 With that all fileids would have been cached when listing the files while browsing to it ... |
this is okay for master (php7.1+) - but needs to be adapted in the backport |
butonic
force-pushed
the
fix-versions-performance
branch
2 times, most recently
from
0bed389 to
60a725f
Compare
mrow4a
left a comment
mrow4a
left a comment
There was a problem hiding this comment.
My worry here is to also not break any IVersionedStorage functionality, meaning versioning for s3. I would also look into adding new function like getById() doing proper thing, instead of adding additional ambiguous flag.
| * This restricts access to a subfolder of the wrapped storage with the subfolder becoming the root folder new storage | ||
| */ | ||
| class Jail extends Wrapper { | ||
| class Jail extends Wrapper /* implements IVersionedStorage */ |
There was a problem hiding this comment.
So it sometimes should implement IVersionedStorage? Sounds dirty
There was a problem hiding this comment.
this is the unfortunately reality of the wrapper pattern
|
@PVince81 done |
|
any update ? @butonic |
|
@butonic please finish this or hand over |
butonic
force-pushed
the
fix-versions-performance
branch
from
60a725f to
703e2c3
Compare
| return null; | ||
| } | ||
| $nodes = $userNode->getParent()->getById($this->getRootId(), true); | ||
| return $nodes[0] ?? null; |
There was a problem hiding this comment.
note to self: will clash on backport due to PHP 5.6 compat
|
went through with @tomneedham and it looks ok.
|
|
So... I spent a few hours adding coverage where coverage was missing and it wasn't easy and my head hurts from too many mocks... Let's hope this will bring the coverage to an acceptable level for merging. See my comments on the remaining items, I'm not sure if covering those is worth it. |
|
I'll need a review on the coverage commit: 5e6e445 @butonic @DeepDiver1975 |
|
@PVince81 omg I love you! |
PVince81
force-pushed
the
fix-versions-performance
branch
from
5e6e445 to
bf6ce2d
Compare
|
fixed code style issues and rebased |
PVince81
force-pushed
the
fix-versions-performance
branch
from
bf6ce2d to
244dfbb
Compare
|
Had to delete again the test for comments/appinfo/app.php uncovered code, because it's unreliable as its state depend on other tests. A proper way would be to first refactor app.php into an Application class and then test the latter. Something which I do not want to start in this PR as it would require regression testing things unrelated to what this PR fixes... |
PVince81
force-pushed
the
fix-versions-performance
branch
from
244dfbb to
3d7f3b0
Compare
|
I've added even more tests, some covering the issues in this PR, I hope this will make us reach the necessary threshold... |
|
stable10: #33859 |
|
it's the second time this new test fails on object store: Sadly there is a non-mockable static method in there which is why I had to use an actual user/storage. No idea why the storage would not be available for object store. |
but first debug the above problem... |
|
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Added more tests for meta node collection Add tests for comments activity listener Add activity listener test for system tags Add test coverage for files' ActivityHelper Test for legacy shares in fed share provider
PVince81
force-pushed
the
fix-versions-performance
branch
from
3d7f3b0 to
15f00cc
Compare
|
I managed to get something close to the object store issue. It turns out that The tests should all pass now. |
|
all green, please review the tests now @DeepDiver1975 @butonic |
|
this now triggered unit test failures in storage-related apps:
will look into this... |
|
ok, the issue is simply that the unit tests in those apps are using |
|
Backport |
Today, the node api currently iterates over all storages when resolving a node by id to return all possible paths to the file. Even when it has found a path it continues to iterate over the other storages in order to find all nodes in the users tree.
That causes some overhead.
Actually, we never really use anything but the first node ... so this pr adds an optional parameter to only return the first node that is found. as you can see, quite a few places can profit.
Furthermore, the Jail Wrapper needs to intercept the IVersionedStorage calls, so it can adjust the paths correctly. However it should not implement the interface AFAICT because the wrapped storages might not actually implement versioning. I may get the logic in instanceOfStorage() wrong but this can be tested.
Finally, the Meta endpoint iterates over all possible storages, because it does not yet use the LazyRootFolder ...
All of this together significantly improves versions performance.
I won't start on the tests until I get a response on the approach. Especially, because I don't inject the UserFolder but fetch if from the $server.
@DeepDiver1975 did you intend the Meta endpoint to be able to access any fileid? then, limiting the versioning meta nodes to versions might not be the right approach. We might have to query the db table directly to immediately lookup storage and path ...