[stable9] Backport lazy init of shared storage/mount #26912

Merged
merged 4 commits into from Jan 18, 2017

Projects

None yet

6 participants

@PVince81
Collaborator
PVince81 commented Jan 10, 2017 edited

Description

  • Backport of 2f1c62c from #23919 to introduce LazyStorageMountInfo
  • Backport of c8a385d from #25789 to have a shortcut to retrieve the numeric storage id without having to initialize the shared storage fully
  • Backport of dc7d55c to have an additional safety against unwanted recursions in shared storages (could not make it happen on this PR but the extra safety won't harm)

Related Issue

Hopefully fixes the issue described in #26702

Motivation and Context

There's a perf regression in 9.0 that didn't exist in 8.2 due to the introduction of oc_mounts.
Perf was improved in 9.1 through #25789 so hopefully backporting a subset of it will also improve performance in 9.0.

How Has This Been Tested?

Steps:

  1. Setup OC 9.0
  2. Run https://gist.github.com/PVince81/4c4c2e938ae29065dcf02cdc2cc6d0bc (requires latest pyocclient)
  3. curl -D - -X GET -u recipient:recipient http://localhost/owncloud/remote.php/webdav/file1.txt > file.txt => ignore the result
  4. curl -D - -X GET -u recipient:recipient http://localhost/owncloud/remote.php/webdav/file1.txt > file.txt, again, and have a look at the value of "Time spent"

Before this fix (v9.0.7): time spent was 7 seconds
After this fix: time spent is down to 4 seconds

Running the same test on v9.1.3 also gives 4 seconds.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

TODOs:

⚠️ it is likely that the introduction of this laziness will cause a regression due to a new recursive behavior (this was observed on 9.1, need to confirm if it will appear as well here through this PR). This was fixed by the PR #25789 which itself brought its own regressions fixed by subsequent PRs.

  • check if there is a simpler shortcut => yes, but might not be worth it if we need additional backports anyway due to laziness (shortcut is on branch "stable9-shortcut-sharedmount-ids")
  • analyze whether this PR now contains the shared storage recursion hell and requires further backports => no recursion observed, but still did an extra safety backport
  • no further backports needed as this is not a Jail instance, so no further regression that were observed on 9.1 would happen
  • @mrow4a please rerun your powerful tests on this branch
@PVince81 PVince81 added this to the 9.0.8 milestone Jan 10, 2017
@mention-bot

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @butonic and @DeepDiver1975 to be potential reviewers.

@PVince81
Collaborator
PVince81 commented Jan 10, 2017 edited

Good news: the SharedStorage in stable9 is not a Jail, so no need to backport the jail fix from 3717e62.

Now there is still a risk that the recursion happens when calling $this->ownerView->getPath(), I'll debug into it to see if it happens. If yes, we'll need dc7d55c

  • also backport the FailedStorage case, for more robustness ? 837bf1c => no
@PVince81
Collaborator

Attempting to find steps to reproduce the recursion in question, because we never really had steps for that... Here #25557 (comment)

@PVince81
Collaborator

Could not reproduce the infinite recursion on this PR here #25557 (comment).

It is likely that the code is too different. But there is still a slight chance that some code paths triggers it, so I decided to backport the recursion safety commit as well as it won't hurt other code paths: dc7d55c

The FailedStorage backport looks unnecessary as the SharedStorage is not a Jail instance like on 9.1 where a lot of logic is different.

I think this PR is now ready for review and testing.

@PVince81
Collaborator

@mrow4a this is ready for testing, please run your magic tests on this branch. Thanks

@PVince81 PVince81 requested review from jvillafanez and mrow4a Jan 10, 2017
@PVince81 PVince81 requested a review from butonic Jan 10, 2017
@PVince81
Collaborator

Looks like e6d8779 breaks some unit tests:

	Test Result (2 failures )

    Test_Files_Sharing_Api.testGetShareFromFolderReReShares
    Test_Files_Sharing_Api.testGetShareFromFileReReShares
@PVince81
Collaborator

This because the tests create shares the old way which doesn't create flat reshares.
Tried to backport 6509220 to have it use the proper APIs, but it makes the tests fail differently now.

@PVince81
Collaborator
PVince81 commented Jan 11, 2017 edited

I had to compile a few backports to make tests pass again. The reason is that the getPath() shortcut is based on the assumption that reshares are flat. But the unit tests use the old API which uses the old chain-like shares. Note that the old API isn't used any more.

  • REQUIRES: this other backport PR #26927 to be merged first before tests can pass here.
@PVince81
Collaborator

Looks like I'll have to figure out how to run the complex performance tests myself...

Can I at least get a code review ?

@PVince81
Collaborator

Also, am running smashbox against this branch, just in case.

@mrow4a
Contributor
mrow4a commented Jan 11, 2017 edited

@PVince81 I have everything set up, OC server with different versions, smashbox appliance etc. I just need to checkout the branch on one of server VM, put correct config files and run the tests on "smashbox" VM.

Everything is there, I will try to finish quickly university stuff and help you -> https://malibu.int.owncloud.com:8006/ on internal network, password you know,

@PVince81
Collaborator

@mrow4a thanks a lot!

- if ($storage->instanceOfStorage('\OC\Files\Storage\Shared')) {
- $rootId = (int)$storage->getShare()['file_source'];
+ // note: SharedMount goes there
+ if (method_exists($mount, 'getStorageRootId')) {
@phisch
phisch Jan 12, 2017 Contributor

Why is this necessary? IMountPoint has this method, if it does not exist you would either get an error for something not implementing IMountPoint correctly, or not injecting an object of type IMountPoint.

@PVince81
PVince81 Jan 12, 2017 Collaborator

IMountPoint only has this method on stable9.1 or master and we don't want to introduce new methods as it qualifies as API change. Other implementers of IMountPoint (ex: external storage) will likely not implement this one in this version. So this hack is there to accomodate for that.

@phisch
phisch Jan 12, 2017 Contributor

Ah, i didn't think about that! Makes sense then!

icewind1991 and others added some commits Apr 15, 2016
@icewind1991 @PVince81 icewind1991 Only construct the storage when we start using it 2b6b2dc
@PVince81 PVince81 Get shared storage numeric id directly from DB
To prevent recursions in initMountPoints which requires the numeric id
to populate oc_mounts
a55da70
@PVince81 PVince81 Flag to not recurse into shared mounts in getPath
32bde45
@PVince81
Collaborator

Rebased to include #26927, tests should pass now.

@PVince81
Collaborator

@mrow4a has just confirmed that this improves performance significantly, and some results aren't exponential any more. The results aren't too close to master, but good enough to be released.

Please review the code @PhilippSchaffrath @jvillafanez

This PR will also need some regression testing @owncloud/qa

apps/files_sharing/lib/sharedmount.php
@@ -58,9 +61,10 @@ class SharedMount extends MountPoint implements MoveableMount {
public function __construct($storage, array $mountpoints, $arguments = null, $loader = null) {
$this->user = $arguments['user'];
$this->recipientView = new View('/' . $this->user . '/files');
- $newMountPoint = $this->verifyMountPoint($arguments['share'], $mountpoints);
+ $this->share = $arguments['share'];
@jvillafanez
jvillafanez Jan 16, 2017 Contributor

We should check if the "share" key is present and throw an error otherwise.

@PVince81
PVince81 Jan 18, 2017 Collaborator

fixed

+ */
+ public function __construct(IUser $user, IMountPoint $mount) {
+ $this->user = $user;
+ $this->mount = $mount;
@jvillafanez
jvillafanez Jan 16, 2017 Contributor

I think we have to call the parent constructor. The parent might not be properly initialized otherwise and could cause issues.
If this is intended, write a comment explaining it.

@PVince81
PVince81 Jan 16, 2017 Collaborator

This was backported from 9.1 which didn't call the parent either (not my code), I hope it had a good reason

@@ -1663,6 +1665,11 @@ public function getPath($id) {
/**
* @var \OC\Files\Mount\MountPoint $mount
*/
+ if (!$includeShares && $mount instanceof SharedMount) {
@jvillafanez
jvillafanez Jan 16, 2017 Contributor

I'm not sure if this will be enough to prevent the infinite loop

@PVince81
PVince81 Jan 16, 2017 Collaborator

This was enough on 9.1, at least for known cases

@jvillafanez
Contributor

I think we'll have to review the lazystoragemountinfo.php the sooner the better. It looks like a bad design, and this kind of things are always difficult to get rid of if they're left alone for a long time.

First I think it's pointless to have both classes (CachedMountInfo and LazyStorageMountInfo): you'll do it lazily or not, but you won't do it lazily sometimes and then normal.

Second, usually the class on top (CachedMountInfo in this case) is the one that has control over what the subclasses are allowed to do, and only a few methods are overwritten in the subclasses. Taking into account that we have control over both classes (CachedMountInfo and LazyStorageMountInfo), there is no reason not to modify the CachedMountInfo accordingly to allow lazyness in a clearer way, instead of overwrite almost all the methods and using protected parameters that aren't documented as "allowed to be used this way" in the subclasses or something similar.

I don't expect to solve this in this PR because it's a backport, but we have to take this into account.

@PVince81
Collaborator

First I think it's pointless to have both classes (CachedMountInfo and LazyStorageMountInfo): you'll do it lazily or not, but you won't do it lazily sometimes and then normal.

You're right. I'm also not too happy about these. Would you suggest merging both classes into a single one ? (in a separate tech debt PR) That would likely solve all points you mentionned (calling parent, always lazy and no weird control)

@jvillafanez
Contributor

Users won't care about the specific implementation as long as it's fast enough, and a don't think there are cases where one implementation would perform better than the other one in order to provide a switch for those cases, so one implementation is enough.

@PVince81 PVince81 Check arguments in SharedMount constructor
b26aa0f
@PVince81
Collaborator

@jvillafanez I added the required checks. Your LazyCacheMountInfo can be adressed separately on master as tech debt: #26960

@jvillafanez
Contributor

👍

@PVince81 PVince81 merged commit 72f0fd6 into stable9 Jan 18, 2017

3 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81 PVince81 deleted the stable9-sharedstoragehell-backport branch Jan 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment