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.

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

Add additional caching in the RootCollection and SMB classes #37451

Merged
merged 2 commits into from
May 28, 2020

Conversation

jvillafanez
Copy link
Member

Description

Optimize access to the SMB storage. The focus has been to optimize the usage of the opendir in the SMB storage because there were several calls to the opendir method:

  • The SMB's opendir method was overriding the class cache. This was a problem because the FileInfo objects has an internal cache for the dos mode. By overriding the cache, we were forced to request again the dos mode to the external SMB server.
    Now we're checking the cache and prioritize that information, which should have the dos mode already cached.
  • Several instances of the FilesHome object were being created. This was likely disturbing the caching mechanism in the dav layer, causing additional calls to the backend (at least one additional call to the opendir method in the SMB backend)
    Now we're returning the same node instance when asked in order to reuse the cache mechanism.

With these changes, it's expected to have only 2 calls to the SMB's opendir method. These 2 calls come from the automatic scan that happen when we get the cache entry (the first one), and when we actually ask for the folder contents (the second one).

Related Issue

https://github.com/owncloud/enterprise/issues/4007

Motivation and Context

How Has This Been Tested?

Manually checking the curl timings. The following results come from the same local environment using 4e04c49 (current master) as base.

----without any change-----
curl -w "@curl-timing-format.txt" -o /dev/null -s -X PROPFIND -u admin:admin 'http://10.0.2.27:6080/remote.php/dav/files/admin/SMB/'
         time_total:  0,389524
         time_total:  0,359545
         time_total:  0,368743
         time_total:  0,354103
         time_total:  0,370374
         time_total:  0,371718
         time_total:  0,343153
         time_total:  0,342084

-------------------------------------
----with just the change in the SMB.php file----
curl -w "@curl-timing-format.txt" -o /dev/null -s -X PROPFIND -u admin:admin 'http://10.0.2.27:6080/remote.php/dav/files/admin/SMB/'
         time_total:  0,324680
         time_total:  0,344775
         time_total:  0,333347
         time_total:  0,333059
         time_total:  0,357924
         time_total:  0,327403
         time_total:  0,304593
         time_total:  0,327727
         time_total:  0,329288

-------------------------------------
----with the whole PR----
juan@juan-VirtualBox:~$ curl -w "@curl-timing-format.txt" -o /dev/null -s -X PROPFIND -u admin:admin 'http://10.0.2.27:6080/remote.php/dav/files/admin/SMB/'
         time_total:  0,283274
         time_total:  0,270603
         time_total:  0,277665
         time_total:  0,281935
         time_total:  0,303008
         time_total:  0,287516
         time_total:  0,264756
         time_total:  0,282469
         time_total:  0,425668
         time_total:  0,278654
         time_total:  0,283069

The performance improvement should be noticeable for the SMB connection. Other backends should also get some benefit due to the cache in the RootCollection.php file, although not tested.

Screenshots (if appropriate):

Number of calls before this PR:
Screenshot from 2020-05-26 18-05-20

Number of class after this PR:
Screenshot from 2020-05-26 18-05-57

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@codecov
Copy link

codecov bot commented May 26, 2020

Codecov Report

Merging #37451 into master will decrease coverage by 0.00%.
The diff coverage is 37.50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #37451      +/-   ##
============================================
- Coverage     64.68%   64.68%   -0.01%     
- Complexity    19331    19333       +2     
============================================
  Files          1277     1277              
  Lines         75512    75524      +12     
  Branches       1331     1331              
============================================
+ Hits          48844    48849       +5     
- Misses        26276    26283       +7     
  Partials        392      392              
Flag Coverage Δ Complexity Δ
#javascript 54.14% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.84% <37.50%> (-0.01%) 19333.00 <0.00> (+2.00) ⬇️
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/Files/RootCollection.php 9.52% <0.00%> (-4.77%) 7.00 <0.00> (+2.00) ⬇️
apps/files_external/lib/Lib/Storage/SMB.php 49.66% <100.00%> (+0.34%) 0.00 <0.00> (ø)
lib/private/License/LicenseManager.php 100.00% <100.00%> (ø) 35.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e04c49...a021f40. Read the comment docs.

@codecov
Copy link

codecov bot commented May 26, 2020

Codecov Report

Merging #37451 into master will decrease coverage by 0.00%.
The diff coverage is 37.50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #37451      +/-   ##
============================================
- Coverage     64.68%   64.68%   -0.01%     
- Complexity    19331    19333       +2     
============================================
  Files          1277     1277              
  Lines         75512    75524      +12     
  Branches       1331     1331              
============================================
+ Hits          48844    48849       +5     
- Misses        26276    26283       +7     
  Partials        392      392              
Flag Coverage Δ Complexity Δ
#javascript 54.14% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.84% <37.50%> (-0.01%) 19333.00 <0.00> (+2.00) ⬇️
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/Files/RootCollection.php 9.52% <0.00%> (-4.77%) 7.00 <0.00> (+2.00) ⬇️
apps/files_external/lib/Lib/Storage/SMB.php 49.66% <100.00%> (+0.34%) 0.00 <0.00> (ø)
lib/private/License/LicenseManager.php 100.00% <100.00%> (ø) 35.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e04c49...6173794. Read the comment docs.

@owncloud owncloud deleted a comment from update-docs bot May 27, 2020
@phil-davis
Copy link
Contributor

Override codecov and merge?

@DeepDiver1975 DeepDiver1975 merged commit 9e93202 into master May 28, 2020
@delete-merged-branch delete-merged-branch bot deleted the smb_optimize branch May 28, 2020 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants