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

Already pass size difference to the cache updater to avoid calculation of the folder size to update #28310

Closed
wants to merge 4 commits into from

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Aug 4, 2021

With S3 the scanner will never return anything so the updater always falls back to a full size calculation. Since we know the transferred filesize during upload, we can pass it as an optional parameter then in order to directly update the parent folder sizes.

Additionally the sizeDifference is also passed in the View when creating directories (which always have a difference of 0) and when calling file_put_contents where we know the size difference from the storage return value.

@juliusknorr juliusknorr added this to the Nextcloud 23 milestone Aug 4, 2021
@juliusknorr juliusknorr requested review from icewind1991 and a team August 4, 2021 13:41
@juliusknorr juliusknorr force-pushed the enh/s3-size-calc branch 2 times, most recently from eb540ed to 720fc91 Compare August 5, 2021 07:38
apps/dav/lib/Connector/Sabre/File.php Outdated Show resolved Hide resolved
@juliusknorr
Copy link
Member Author

Pushed a fix for the comment and an additional commit for another case where we can speed up updating the size:

Additionally the sizeDifference is also passed in the View when creating directories (which always have a difference of 0) and when calling file_put_contents where we know the size difference from the storage return value. This avoids an unnecessary size calculation when creating user avatars where it would have calculated the size of the appdata_*/avatar folder for each user creation call which would cause a high read load on databases with lots of users.

@juliusknorr juliusknorr requested review from artonge and Pytal September 2, 2021 18:46
lib/private/Files/View.php Outdated Show resolved Hide resolved
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Besides @Pytal's change request, it is fine by me

Copy link
Member

@Pytal Pytal left a comment

Choose a reason for hiding this comment

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

Yep, other than the comment above, good with me :)

@blizzz
Copy link
Member

blizzz commented Sep 9, 2021

CI is unhappy

@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 9, 2021
@skjnldsv skjnldsv mentioned this pull request Oct 13, 2021
@skjnldsv skjnldsv modified the milestones: Nextcloud 23, Nextcloud 24 Oct 22, 2021
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
@blizzz blizzz mentioned this pull request Mar 31, 2022
This was referenced Apr 7, 2022
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
This was referenced Aug 12, 2022
This was referenced Aug 24, 2022
This was referenced Sep 6, 2022
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
This was referenced Sep 20, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@juliusknorr juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 14, 2022
@@ -266,7 +267,7 @@
// because we have no clue about the cause we can only throw back a 500/Internal Server Error
throw new Exception($this->l10n->t('Could not write file contents'));
}
[$count, $result] = \OC_Helper::streamCopy($data, $target);
[$writtenByteCount, $result] = \OC_Helper::streamCopy($data, $target);

Check notice

Code scanning / Psalm

PossiblyInvalidArgument

Argument 2 of OC_Helper::streamCopy expects resource, possibly different type resource|true provided
try {
$count = $partStorage->writeStream($internalPartPath, $wrappedData);
$writtenByteCount = $partStorage->writeStream($internalPartPath, $wrappedData);

Check notice

Code scanning / Psalm

PossiblyInvalidArgument

Argument 2 of OCP\Files\Storage\IWriteStreamStorage::writeStream expects resource, possibly different type bool|resource provided
@szaimen
Copy link
Contributor

szaimen commented Oct 16, 2022

Still CI failures...

@juliusknorr
Copy link
Member Author

Restarted drone as I cannot see the failure locally and also can't see a relation to the change.

@szaimen
Copy link
Contributor

szaimen commented Oct 17, 2022

failing tests look related to me...

@juliusknorr
Copy link
Member Author

Sorry, read the output wrong, will have a look

…lation on s3

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
…p correcting the folder size

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@blizzz blizzz mentioned this pull request Feb 1, 2023
@juliusknorr
Copy link
Member Author

I haven't found a proper way yet to pass this along. I'll close this for now, but will keep a note to probably discuss further for future performance optimizations.

@skjnldsv skjnldsv deleted the enh/s3-size-calc branch March 14, 2024 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants