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

fix(performance): Do not set up filesystem on every call #36589

Merged
merged 5 commits into from
Feb 20, 2023
Merged

fix(performance): Do not set up filesystem on every call #36589

merged 5 commits into from
Feb 20, 2023

Conversation

miaulalala
Copy link
Contributor

@miaulalala miaulalala commented Feb 7, 2023 •
edited by juliusknorr
Loading

Removed old OO_FileChunking logic that produced GC- collectable chunks

Added BG Job that will run a garbage collection for every user

Signed-off-by: Anna Larch anna@nextcloud.com

Summary

Comparison for OCS requests:
https://blackfire.io/profiles/compare/2c3c3699-137e-49a3-a363-e0950bb278d4...41a33cf0-71ee-42aa-9791-9ef74f99e6c3/graph?settings%5Bdimension%5D=wt&settings%5Bdisplay%5D=landscape&settings%5BtabPane%5D=nodes&selected=&callname=main()&constraintDoc=
Screenshot 2023-02-16 at 16 52 52

TODO

  • Testing

Checklist

@miaulalala miaulalala self-assigned this Feb 7, 2023
@miaulalala miaulalala marked this pull request as draft February 7, 2023 14:57
icewind1991
icewind1991 previously requested changes Feb 7, 2023
View reviewed changes
lib/base.php Show resolved Hide resolved
@miaulalala miaulalala force-pushed the enh/perf-remove-icache branch from a0a28a9 to aadd950 Compare February 7, 2023 17:01
come-nc
come-nc approved these changes Feb 7, 2023
View reviewed changes
apps/dav/lib/Connector/Sabre/Directory.php Show resolved Hide resolved
apps/dav/lib/Connector/Sabre/Directory.php Outdated Show resolved Hide resolved
come-nc
come-nc reviewed Feb 7, 2023
View reviewed changes
lib/private/Server.php Outdated Show resolved Hide resolved
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 7, 2023
View reviewed changes
apps/files/lib/BackgroundJob/FileChunkCleanupJob.php Fixed Show fixed Hide fixed
apps/files/lib/BackgroundJob/FileChunkCleanupJob.php Fixed Show fixed Hide fixed
apps/files/lib/BackgroundJob/FileChunkCleanupJob.php
public function run($argument) {
$this->userManager->callForSeenUsers(function (IUser $user): void {
$this->logger->debug('Running chunk cleanup job for user '. $user->getUID());
$fileCache = new File();

Check notice

Code scanning / Psalm

DeprecatedClass

OC\Cache\File is marked deprecated
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 7, 2023
View reviewed changes
apps/files/lib/BackgroundJob/FileChunkCleanupJob.php
/**
* This job cleans up all backups except the latest 3 from the updaters backup directory
*/
public function run($argument): void {

Check notice

Code scanning / Psalm

MissingParamType

Parameter $argument has no provided type
@miaulalala miaulalala marked this pull request as ready for review February 7, 2023 19:35
@miaulalala miaulalala force-pushed the enh/perf-remove-icache branch from ac9d919 to 60f1f7f Compare February 7, 2023 19:48
@miaulalala miaulalala force-pushed the enh/perf-remove-icache branch from a7e914a to a7b96c5 Compare February 8, 2023 13:26
@juliusknorr juliusknorr mentioned this pull request Feb 9, 2023
Merged
4 tasks
@miaulalala miaulalala added pending documentation This pull request needs an associated documentation update 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 9, 2023
@miaulalala miaulalala changed the title Do not set up filesystem on every call fix(performance): Do not set up filesystem on every call Feb 9, 2023
@miaulalala miaulalala force-pushed the enh/perf-remove-icache branch from a90f63e to 0b25e61 Compare February 9, 2023 15:08
@juliusknorr juliusknorr added this to the Nextcloud 26 milestone Feb 9, 2023
@juliusknorr juliusknorr force-pushed the enh/perf-remove-icache branch from 6473d0d to 1a7041e Compare February 9, 2023 21:44
@juliusknorr juliusknorr force-pushed the enh/perf-remove-icache branch 2 times, most recently from 756cc5e to 270828b Compare February 10, 2023 10:54
@miaulalala
Copy link
Contributor Author

/rebase

@juliusknorr juliusknorr force-pushed the enh/perf-remove-icache branch 2 times, most recently from 15b85a1 to 248c224 Compare February 14, 2023 21:02
@juliusknorr
Copy link
Member

Last commit seems to indeed break subfolder listing of mount points :/ Needs another dive into why Depth > 0 requests for dav fail with shares being moved into those subfolders without that then.

miaulalala and others added 5 commits February 17, 2023 19:18
@miaulalala @juliusknorr
Do not set up filesystem on every call
5d4efb4 
Also remove old Oc_FileChunking logis that produced GC- collectable chunks

Signed-off-by: Anna Larch <anna@nextcloud.com>
@juliusknorr
tests: Fix test isolation on object storage
20058eb 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
tests: Remove legacy checksum test
3493099 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
tests: Remove legacy chunking tests
ec35650 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
fix: Use passed LazyFolder when building sabre nodes
3bdf332 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the enh/perf-remove-icache branch from 248c224 to 3bdf332 Compare February 17, 2023 18:21
@juliusknorr juliusknorr mentioned this pull request Feb 18, 2023
Merged
@juliusknorr
Copy link
Member

I finally figured out why the failure was happening. When using the nodes api in dav we initialise it with the wrong view fake root /user/files instead of ``. This doesn't cause issues as dav rarely calls nodes api endpoints that then access the view. Pushed a fix in this PR to reuse already passed LazyFolder instances. The full fix is separate in #36774

@juliusknorr juliusknorr merged commit bba3a1c into master Feb 20, 2023
@juliusknorr juliusknorr deleted the enh/perf-remove-icache branch February 20, 2023 07:59
This was referenced Feb 21, 2023
Closed
Merged
@DaphneMuller
Copy link

hello @miaulalala
Thank you so much for your work on this pull request! This ticket seems to have the tag 'pending documentation'.
Is there any chance you could clarify what documentation is missing? Is this for admins or for app developers?

@miaulalala
Copy link
Contributor Author

This PR was reverted @DaphneMuller

@juliusknorr juliusknorr mentioned this pull request Feb 21, 2023
Open
1 task
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
Merged
@ChristophWurst ChristophWurst removed the pending documentation This pull request needs an associated documentation update label Mar 16, 2023
@icewind1991 icewind1991 mentioned this pull request Jun 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@come-nc come-nc come-nc approved these changes

@juliusknorr juliusknorr juliusknorr approved these changes

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 is a code owner

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

Assignees

@miaulalala miaulalala

Labels
3. to review Waiting for reviews performance 🚀
Projects
None yet
Milestone
Nextcloud 26
Development

Successfully merging this pull request may close these issues.

Basic auth requests always set up the filesystem
6 participants
@miaulalala @juliusknorr @DaphneMuller @icewind1991 @ChristophWurst @come-nc