Reduce memory footprint of trash expiry jobs#36602

mrow4a · 2019-12-17T22:24:07Z

Related issue: https://github.com/owncloud/enterprise/issues/2925

Main problem: PHP PDO driver leaks memory - https://www.dragonbe.com/2015/07/speeding-up-database-calls-with-pdo-and.html when used in long running jobs (php pdo works well in online request where scrips live short). We cannot have scrollable PDO cursor (and it is not good idea either sometimes), and thus whole resultset is returned to memory causing memory allocation peak, when calling execute. This causes memory to cumulate and memory allocated to script is not released in long running jobs like trash expiry.

We also steadily leak memory in below when run 1000 times for 1000 users.

\OC_Util::tearDownFS();
\OC_Util::setupFS($user);

Solution:

reduce memory footprint where possible for single user
In order not to keep memory when it is not used after resulting large result set, PHP script has to terminate. Thus, do not process all users at once within 1 PHP script, we need to chunk expiry for users (chunk according to max number of files processed for user?)
do not fetch trashbin locations for background jobs (as it is not used)
split trashbin manager (online queries mostly) and trash expiry manager (background jobs only) to optimize for each case
do not keep memory for results of functions that potentially can return large results sets and keep them for duration of function, causing memory to grow further
callForUser with limit and offset to allow page users
add user/account unit tests
do not allow cron to process all the users at once (run many php crons instead, chunk users)
add trashbin expiry unit tests

@DeepDiver1975 @jvillafanez @PVince81

mrow4a · 2019-12-18T00:39:26Z

@jvillafanez @DeepDiver1975 @pako81 will 500 users per session be good? The same we have for FileScan - https://github.com/owncloud/core/blob/master/apps/files/lib/BackgroundJob/ScanFiles.php#L98

tests/lib/User/AccountMapperTest.php

codecov · 2019-12-28T19:59:22Z

Codecov Report

Merging #36602 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #36602   +/-   ##
=========================================
  Coverage     64.69%   64.69%           
- Complexity    19114    19127   +13     
=========================================
  Files          1269     1270    +1     
  Lines         74808    74843   +35     
  Branches       1327     1327           
=========================================
+ Hits          48395    48423   +28     
- Misses        26022    26029    +7     
  Partials        391      391

Flag	Coverage Δ	Complexity Δ
#javascript	`54.10% <ø> (ø)`	`0.00 <ø> (ø)`	⬆️
#phpunit	`65.87% <87.28%> (+<0.01%)`	`19127.00 <44.00> (+13.00)`	⬇️

Impacted Files	Coverage Δ	Complexity Δ
apps/files_trashbin/lib/AppInfo/Application.php	`36.84% <0.00%> (-31.58%)`	`2.00% <0.00%> (ø%)`
apps/files_trashbin/lib/TrashExpiryManager.php	`100.00% <0.00%> (ø)`	`13.00% <0.00%> (?%)`
lib/private/User/AccountMapper.php	`83.20% <0.00%> (+2.85%)`	`36.00% <0.00%> (+5.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 1bd1099...c779815. Read the comment docs.

mrow4a · 2019-12-30T13:55:03Z

@DeepDiver1975 @jvillafanez MR is ready.

lib/private/User/AccountMapper.php

jvillafanez · 2020-01-15T11:42:03Z

lib/private/User/AccountMapper.php

 		}
+
+		if ($limit !== null || $offset !== null) {
+			$qb->orderBy('user_id'); // needed for predictable limit & offset


I think it's better to switch to the (account) "id" instead of the "user_id". If a user is added or removed between runs, this could lead to a user being checked twice or more while another user being neglected for a long time.
Due to the id being an autoincrement column, it's expected that new users will always be added in the last position, not in between.

In addition, we can change the "offset" to "initialId".
Instead of SELECT * FROM table WHERE ..... LIMIT limit OFFSET offset we can use SELECT * FROM table WHERE id > initialID ...... LIMIT limit and store the last id for the next run. This way, if a user is removed we won't rescan a previously-scanned user. In addition, we can take advantage of DB indexes

I think it is better to order by user_id, at least in the scope of this PR. Please have a look in another places in the class, and there we order by user_id (for some reason it was done, and I would not change it before some consultation). For the second comment, I commented in #36602 (comment).

as long as the sorting column is indexed, I don't think it's relevant if you sort by id or user_id. My point is that the "offset" clause is bad and we should get rid of it (https://www.eversql.com/faster-pagination-in-mysql-why-order-by-with-limit-and-offset-is-slow/)

Taking into account that this is a new function, it could be a good starting point. Maybe the change is bigger than I think, so if you think the change is too big for the PR, we can go with the current code for now.

@jvillafanez I am not sure if this is a good idea in this case to expose rowid/accountids for limit, as we use IUserManager->callForUsers public interface - limit and offset are already used in this class in 3 other functions. If this was some internal implementation to page data I would be more then opting for this solution.

I know it is super tempting in this case, but it might happen that other use-cases would be interested in non-arbitrary pages (e.g. you have UI with 100 users per page, and 1000 pages, and I want to calculate something for a page no. 4, how do you do that with the offset being some magic number?).

I'm willing to discuss this offline. Anyway, we can keep the limit + offset approach in this case.

jvillafanez · 2020-01-15T12:10:16Z

apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php

+			$this->config->setAppValue('files_trashbin', 'cronjob_trash_expiry_offset', 0);
+		} else {
+			$offset += self::USERS_PER_SESSION;
+			$this->config->setAppValue('files_trashbin', 'cronjob_trash_expiry_offset', $offset);


As said (below?), it might be better to store the last account id checked so we can start over from there the next run, in order to have better consistency if users are added or removed.

I do not think it is a good idea. In that class we already use offset and limit, also FileScan uses similar mechanism - https://github.com/owncloud/core/blob/master/apps/files/lib/BackgroundJob/ScanFiles.php#L98. I would not over-engineer it here if this is not needed.

I refactored this part of the code to always remember last scanned user. If users are added, no expiration would be needed to be done. If user is removed, there wont be anything to expire probably.

apps/files_trashbin/lib/Command/Expire.php

jvillafanez · 2020-01-15T12:40:56Z

apps/files_trashbin/lib/TrashExpiryManager.php

+		return $this->expiration->isEnabled();
+	}
+
+	public function expiryByRetentionEnabled() {


This might need some docs. This method name is confusing, and I'm not sure when I should use this method or the expiryEnabled one

jvillafanez · 2020-01-15T12:51:58Z

apps/files_trashbin/lib/TrashExpiryManager.php

+			if ($this->expiration->isExpired($trashbinEntry->getMtime())) {
+				Trashbin::delete($trashbinEntry->getName(), $uid, $trashbinEntry->getMtime());
+				$this->logger->info(
+					'Remove "' . $trashbinEntry->getName() . '" from "' . $uid . '" trashbin because it exceeds max retention obligation term.',


String interpolation is preferred over concatenation (https://blog.blackfire.io/php-7-performance-improvements-encapsed-strings-optimization.html). It also might be easier to read.

but not in this line :)

Refactored in a class

apps/files_trashbin/lib/TrashExpiryManager.php

jvillafanez · 2020-01-15T13:18:01Z

apps/files_trashbin/lib/TrashExpiryManager.php

+				);
+
+				// unset the deleted file as it was already processed
+				unset($remainingUserTrashbinContent[$key]);


Confirm this works as expected. Modifying the list you're looping causes problems in most of the languages.
I think it's works because you're looping over a copy of the array, not over the array itself, so modifying the array won't affect the loop.

I refactored it so it is more clear that this is a associative array (unset just removed the index key from array, that still later can be iterated with foreach)

before: [ 0 => a, 1=>b, 2=>c]
after: [ 0 => a, 2=>c]

jvillafanez · 2020-01-15T13:24:17Z

apps/files_trashbin/lib/Trashbin.php

 	/**
 	 * @return Quota
 	 */
 	protected static function getQuota() {


I don't think this method is useful. Either move the code to where it's being used or clarify in the phpdocs that the method will always return a new Quota instance.

It is being used just a function above https://github.com/owncloud/core/blob/v10.3.2/apps/files_trashbin/lib/Trashbin.php#L778. Lets keep it in the class that it is used at.

the function is being used, but what's the point of a function that just creates a new object? Instead of doing self::getQuota() you can do new Quota(.....).
Right now, I don't see any benefit: the function is only used in one place (it isn't being reused) and you can't mock it in the unittests. Note that this might be confusing in the long run because 95% of the "get" function will return always the same instance, they won't create a new instance each time they're called.

micbar

Please add changelog item

mrow4a · 2020-01-26T14:31:09Z

@jvillafanez @micbar @phil-davis while reviewing, please note that also refactor of trashbin manager was motivated to be able to test it with unit tests properly and cover whole code there. I also did test expiration by background jobs (both space and retention) manually.

…or memory

jvillafanez

Just a little detail, but I can live with it.

jvillafanez · 2020-01-28T08:38:45Z

apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php

+	/**
+	 * Tear down owner fs
+	 */
+	protected function tearDownFS() {


this isn't really needed. Just move the \OC_Util::tearDownFS() call inside the run method

jvillafanez · 2020-01-28T08:57:38Z

lib/private/User/AccountMapper.php

 		}
+
+		if ($limit !== null || $offset !== null) {
+			$qb->orderBy('user_id'); // needed for predictable limit & offset


I'm willing to discuss this offline. Anyway, we can keep the limit + offset approach in this case.

jvillafanez · 2020-01-28T09:04:17Z

apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php

+
+			// increase the offset to know where to start next
+			// e.g. in case of crash of this job due to memory
+			$this->config->setAppValue('files_trashbin', 'cronjob_trash_expiry_offset', $offset + $usersScanned);


Just saying, you can sell this better as a transactional mechanism to deal with a memory crash.

mrow4a self-assigned this Dec 17, 2019

mrow4a mentioned this pull request Dec 18, 2019

Add limit and offset for callForAllUsers #36612

Closed

DeepDiver1975 requested changes Dec 19, 2019

View reviewed changes

tests/lib/User/AccountMapperTest.php Outdated Show resolved Hide resolved

mrow4a force-pushed the issue_e2925_2 branch from 0d32490 to 04e27f3 Compare December 22, 2019 18:43

mrow4a marked this pull request as ready for review December 22, 2019 18:46

mrow4a requested review from DeepDiver1975 and jvillafanez December 22, 2019 18:47

mrow4a force-pushed the issue_e2925_2 branch 4 times, most recently from 492cae5 to 7d006b9 Compare December 28, 2019 19:59

mrow4a force-pushed the issue_e2925_2 branch 4 times, most recently from 454f6d1 to 5fe6767 Compare December 30, 2019 12:50

jvillafanez reviewed Jan 15, 2020

View reviewed changes

micbar suggested changes Jan 21, 2020

View reviewed changes

mrow4a force-pushed the issue_e2925_2 branch from 5fe6767 to 093f0c2 Compare January 24, 2020 17:21

mrow4a mentioned this pull request Jan 24, 2020

Make sure to filter filecache subfolders in sql query instead of memory #36565

Merged

mrow4a force-pushed the issue_e2925_2 branch from 093f0c2 to 1b7c27a Compare January 26, 2020 14:26

mrow4a added 2 commits January 26, 2020 15:32

Do not add extraData to trashbin files for maintenance jobs

cdb8a93

Allow to offset and limit on callForUsers

6b60760

mrow4a force-pushed the issue_e2925_2 branch from 1b7c27a to 4404687 Compare January 26, 2020 15:23

Split trash manager and trash expiry manager, optimize trash expiry f…

c779815

…or memory

mrow4a force-pushed the issue_e2925_2 branch from 4404687 to c779815 Compare January 27, 2020 19:11

jvillafanez approved these changes Jan 28, 2020

View reviewed changes

jvillafanez reviewed Jan 28, 2020

View reviewed changes

micbar self-requested a review January 28, 2020 09:22

DeepDiver1975 approved these changes Jan 28, 2020

View reviewed changes

micbar approved these changes Jan 28, 2020

View reviewed changes

micbar merged commit daf13e2 into master Jan 28, 2020

delete-merged-branch bot deleted the issue_e2925_2 branch January 28, 2020 09:48

mrow4a mentioned this pull request Apr 19, 2020

Adjust expire trash background job interval #37277

Merged

Conversation

mrow4a commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrow4a commented Dec 18, 2019

Uh oh!

Uh oh!

codecov bot commented Dec 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mrow4a commented Dec 30, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrow4a Jan 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micbar left a comment

Choose a reason for hiding this comment

Uh oh!

mrow4a commented Jan 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvillafanez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

mrow4a commented Dec 17, 2019 •

edited

Loading

codecov bot commented Dec 28, 2019 •

edited

Loading

mrow4a Jan 27, 2020 •

edited

Loading

mrow4a commented Jan 26, 2020 •

edited

Loading