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.

Sign up for GitHub

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

Jump to bottom

Stat cache for DAV storage, improves s2s performance a little bit #14120

Merged
merged 1 commit into from
Apr 8, 2015
Merged

Stat cache for DAV storage, improves s2s performance a little bit #14120

merged 1 commit into from
Apr 8, 2015

Conversation

PVince81
Copy link
Contributor

@icewind1991 @karlitschek @DeepDiver1975 @schiesbn

I ran the pyocclient test suite:

  1. Against s2s folder on localhost with this PR: 539.394s
  2. Against s2s folder on localhost without this PR: 964.598s
  3. Against regular folder: 71.972s

@PVince81
Copy link
Contributor Author

Partial fix for #13882

@PVince81
Copy link
Contributor Author

Regression testing (for the stuff that might not be covered)

  • manual test with s2s shares, web UI
  • test with regular DAV/ownCloud ext storage
  • regression test sync client + s2s share

@karlitschek
Copy link
Contributor

nice idea. 👍 Not tested yet.

@PVince81
Copy link
Contributor Author

Well ideally we need this for every ext storage, see #7910

@PVince81
Copy link
Contributor Author

This improves performance from about 1.78 times, see #14120 (comment)

@PVince81 PVince81 mentioned this pull request Feb 11, 2015
Closed
nickvergessen
nickvergessen reviewed Feb 24, 2015
View reviewed changes
lib/private/cache/arraycache.php
@@ -0,0 +1,52 @@
<?php
/**
* Copyright (c) 2014 Andreas Fischer <bantu@owncloud.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

accident or fact?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fact. Borrowed from #7897 which also needs review since many releases...

@nickvergessen nickvergessen added this to the 8.1-next milestone Feb 24, 2015
nickvergessen
nickvergessen reviewed Feb 26, 2015
View reviewed changes
lib/private/app.php.rej
@@ -0,0 +1,24 @@
--- lib/private/app.php
Copy link
Contributor

Choose a reason for hiding this comment

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

accident?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah...

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 4, 2015

  • BUG: cache path mismatch due to leading slashes

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 4, 2015

Additional fails to look into:


1) Test\Files\Storage\OwnCloud::testStat
Failed asserting that 100 matches expected 1425463750.

/srv/www/htdocs/owncloud/tests/lib/files/storage/storage.php:301

2) Test\Files\Storage\OwnCloud::testRecursiveRmdir
Failed asserting that true is false.

/srv/www/htdocs/owncloud/tests/lib/files/storage/storage.php:359

3) Test\Files\Storage\OwnCloud::testRecursiveUnlink
Failed asserting that true is false.

/srv/www/htdocs/owncloud/tests/lib/files/storage/storage.php:372

4) Test\Files\Storage\OwnCloud::testRenameDirectory
Failed asserting that true is false.

/srv/www/htdocs/owncloud/tests/lib/files/storage/storage.php:438

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 4, 2015

  • recursively remove cached entries on delete...

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 4, 2015

@icewind1991 sometimes I wonder if we shouldn't just wire up storage file operation directly to the DB cache instead of redoing additional calls or introducing additional stat cache layers...

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 4, 2015

For example here: https://github.com/owncloud/core/blob/master/lib/private/files/storage/dav.php#L507
That is_dir call could be done against the view / DB cache instead of connecting to the remote. It is just unfortunate that we're at storage level here, not view level.

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 4, 2015

  • test: storage not available / storage expired cases

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 4, 2015

  • test: DAV unit tests

@icewind1991
Copy link
Contributor

@icewind1991 sometimes I wonder if we shouldn't just wire up storage file operation directly to the DB cache instead of redoing additional calls or introducing additional stat cache layers...

imo that should not be done in the storage layer, ideally apps use the Node/FileInfo objects to get stuff directly from cache but we could do it in the View to for some operations.

@icewind1991
Copy link
Contributor

Also note that flysystem has integrated caching support so for every backend using that (webdav is an option) we get stat caching for free

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 4, 2015

I'm not talking about apps. Sometimes the storage itself needs to do checks like is_dir, etc. But that won't be cached by default. We don't want storage classes to use the node API (going up through the layers)

@MorrisJobke
Copy link
Contributor

rebase needed.

What is the state of this?

@PVince81
Copy link
Contributor Author

It's should work but needs testing.

@PVince81
Copy link
Contributor Author

  • unit test
  • pyocclient test in OC ext storage
  • pyocclient test in s2s
  • sync client + OC ext storage
  • sync client + s2s folder

BUG: overwrite a file locally with OC ext storage + sync client, file is marked as conflicted! (s2s is fine though) Raised as #15125 (unrelated to this PR)

@PVince81
Copy link
Contributor Author

Requires #15126 to be able to properly regression test unavailable storages

@PVince81
Copy link
Contributor Author

#15126 was merged, rebasing.

@MorrisJobke
Copy link
Contributor

Added stat cache for DAV storage
670ca68 
The stat cache stored known states of files/folders to avoid requerying
the DAV server multiple times.
@PVince81
Copy link
Contributor Author

Now using the existing ArrayCache

@scrutinizer-notifier
Copy link

The inspection completed: No issues found

@PVince81 PVince81 mentioned this pull request Mar 27, 2015
Closed
@ghost
Copy link

ghost commented Apr 7, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/11280/
🚀 Test PASSed.🚀
chuck

@MorrisJobke
Copy link
Contributor

works 👍

@PVince81
Copy link
Contributor Author

PVince81 commented Apr 8, 2015

Second reviewer @icewind1991 @nickvergessen ?

@karlitschek
Copy link
Contributor

👍

MorrisJobke added a commit that referenced this pull request Apr 8, 2015
@MorrisJobke
Merge pull request #14120 from owncloud/dav-statcache
4e60b81 
Stat cache for DAV storage, improves s2s performance a little bit
@MorrisJobke MorrisJobke merged commit 4e60b81 into master Apr 8, 2015
@MorrisJobke MorrisJobke deleted the dav-statcache branch April 8, 2015 19:09
@PVince81 PVince81 mentioned this pull request May 11, 2015
Closed
8 tasks
@PVince81 PVince81 mentioned this pull request Aug 26, 2015
Closed
@lock lock bot locked as resolved and limited conversation to collaborators Aug 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
3 - To Review
Projects
None yet
Milestone
8.1
Development

Successfully merging this pull request may close these issues.
6 participants
@PVince81 @karlitschek @icewind1991 @MorrisJobke @scrutinizer-notifier @nickvergessen