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

Cache results of `normalizePath` #13235

Merged
merged 1 commit into from Jan 10, 2015

Conversation

Projects
None yet
6 participants
@LukasReschke
Copy link
Member

LukasReschke commented Jan 10, 2015

normalizePath is a rather expensive operation and called multiple times for a single path for every file related operation.

In my development installation with about 9GB of data and 60k files this leads to a performance boost of 24% - in seconds that are 1.61s (!) - for simple searches. With more files the impact will be even more noticeable. Obviously this affects every operation that has in any regard something to do with using OC\Files\Filesystem.

The following trace is from a request to /index.php/search/ajax/search.php?query=test&inApps%5B%5D=files&page=1&size=30:

screen shot 2015-01-10 at 11 14 17
performance-win

Part of #13221

@karlitschek @DeepDiver1975 @MorrisJobke FYI

Cache results of `normalizePath`
`normalizePath` is a rather expensive operation and called multiple times for a single path for every file related operation.

In my development installation with about 9GB of data and 60k files this leads to a performance boost of 24% - in seconds that are 1.86s (!) - for simple searches. With more files the impact will be even more noticeable. Obviously this affects every operation that has in any regard something to do with using OC\Files\Filesystem.

Part of #13221
@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Jan 10, 2015

The inspection completed: No new issues

@LukasReschke

This comment has been minimized.

Copy link
Member Author

LukasReschke commented Jan 10, 2015

@icewind1991 As well

@MorrisJobke

This comment has been minimized.

Copy link
Member

MorrisJobke commented Jan 10, 2015

Search still works, also I didn't noticed any errors, while browsing and uploading to the instance.

Code looks good 👍

@owncloud-bot

This comment has been minimized.

Copy link
Contributor

owncloud-bot commented Jan 10, 2015

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

@karlitschek

This comment has been minimized.

Copy link
Member

karlitschek commented Jan 10, 2015

makes sense 👍

MorrisJobke added a commit that referenced this pull request Jan 10, 2015

@MorrisJobke MorrisJobke merged commit 74d1a9e into master Jan 10, 2015

1 check passed

default Merged build finished.
Details

@MorrisJobke MorrisJobke deleted the cache-normalize-path branch Jan 10, 2015

@MorrisJobke MorrisJobke added this to the 8.0-current milestone Jan 10, 2015

@@ -713,6 +715,12 @@ static public function hasUpdated($path, $time) {
* @return string
*/
public static function normalizePath($path, $stripTrailingSlash = true, $isAbsolutePath = false) {
$cacheKey = $path.'-'.-$stripTrailingSlash.'-'.$isAbsolutePath;

This comment has been minimized.

@nickvergessen

nickvergessen Jan 13, 2015

Contributor

Is -$stripTrailingSlash intentional? Looks wrong and might confuse in the future?

This comment has been minimized.

@LukasReschke

LukasReschke Jan 13, 2015

Author Member

All hail to the typo. – Thanks for spotting. Shouldn't produce any bugs but was certainly not intended.

This comment has been minimized.

@LukasReschke

LukasReschke Jan 13, 2015

Author Member

Tada: #13305

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment