OCC scanner commit in batches #27527

PVince81 · 2017-03-28T17:22:53Z

Description

Autocommit after 10k entries when running occ scanner to avoid excessive memory usage.

Related Issue

Fixes #27516

Motivation and Context

How Has This Been Tested?

Manually

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

@jvillafanez @butonic

also see #27516 (comment) for potential concerns.

mention-bot · 2017-03-28T17:22:54Z

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @butonic and @oparoz to be potential reviewers.

PVince81 · 2017-03-28T17:24:35Z

also @mmattel might be interested too

mmattel · 2017-03-28T19:23:55Z

Idea, can also be done in a second step, but would like to see it together:
There are usually 2 triggers, quantity and time.
Proposal, have a second trigger to do the DB commit based on time. Lets say every 4-5 seconds. Either one of the two will fire first.
Beneft:
over fast wires, the commit will be triggered by quantity,
over slow wires, the commit will be triggered by time.

mmattel · 2017-03-28T19:48:42Z

Maybe helps to fix: #25029
The root cause was never identified. But the quantities in the issue "smell" that it could be related.

jvillafanez · 2017-03-29T08:29:30Z

lib/private/Files/Utils/Scanner.php

@@ -45,6 +45,8 @@
 * @package OC\Files\Utils
 */
 class Scanner extends PublicEmitter {
+	const MAX_ENTRIES_TO_COMMIT = 10000;


Has it been tested with a low memory limit, let's say 255MB or less?. It looks too big to me.

tried locally without this patch with 47k files, did not run into the memory issue unfortunately.

In fact, in all of my tests I never ran into a memory issue. I'll redo one one of these days as scanning 100k files can take many many hours...

When you run the test, would you be able to check the memory consumption and share the result?
The consumption should show a pattern like a sawtooth.
Such a result would be eg informative in the documenation.

@mmattel what tools do you recommend to make sure measurement ? Or just log memory usage to the ownCloud log ?

Imho, owncloud log and the use of php memory consumption methods would be sufficient.
We need to get a guideline.
Currently we are flying blind...

jvillafanez · 2017-03-29T08:30:56Z

lib/private/Files/Utils/Scanner.php

@@ -69,6 +85,7 @@ public function __construct($user, $db, ILogger $logger) {
 		$this->logger = $logger;
 		$this->user = $user;
 		$this->db = $db;
+		$this->useTransaction = !(\OC::$server->getLockingProvider() instanceof DBLockingProvider);


This might need a comment. It looks weird that you're using transactions when you aren't using DB locking.

this was there before I just changed the way the state is stored.

no idea why this condition exists... might need some git archeology

jvillafanez · 2017-03-29T08:32:09Z

lib/private/Files/Utils/Scanner.php

+		$this->triggerPropagator($storage, $internalPath);
+		$this->entriesToCommit++;
+		if ($this->useTransaction) {
+			$propagator = $storage->getPropagator();


you can move this inside the if. No need to get a propagator if it isn't going to be used.

you're right, also no need to increase entriesToCommit if we don't use it.
Will move both deeper.

jvillafanez · 2017-03-29T08:46:13Z

lib/private/Files/Utils/Scanner.php

@@ -236,5 +252,20 @@ public function scan($dir = '') {
 	private function triggerPropagator(IStorage $storage, $internalPath) {
 		$storage->getPropagator()->propagateChange($internalPath, time());
 	}
+
+	private function postProcessEntry(IStorage $storage, $internalPath) {


I wonder if we can make the function easier. The overall approach is to open a transaction, scan files and finally commit the transaction. The problem I see with the function is that it doesn't follow the same approach, but instead do a scan (in fact, propagate the change), commit a transaction and open a new one, which makes the process difficult to follow.

Maybe a conditional open transaction, scan and finally conditional commit is easier to follow. The only thing to be careful about should be the opening transaction and the last commit, which aren't conditional.

can you elaborate on what you mean with "conditional open transaction" ? I don't really follow.

Note that one thing we do not want to do is commit at the end of a folder because said folder could contain more than 10k entries or so.

The function would something along these lines (I don't expect the db->inTransaction works, but I think there is something similar)

if ($this->entriesToCommit === 0 && !$this->db->inTransaction) { $this->db->beginTransaction(); } // do whatever you need if ($this->entriesToCommit >= self::MAX_ENTRIES_TO_COMMIT && $this->db->inTransaction) { $this->db->commit(); }

the problem with your proposed change is that we also need to include the propagator's transaction, which somehow doesn't fit with the $this->db->inTransaction() approach. While both are supposed to be in sync, I find that having this specific condition using inTransaction more confusion.

Additionally we also need to check $this->useTransaction in the if statements to find out whether we should at all use transactions.

How about removing the the first and last "transactions" (the first beginTransaction and the last commit) and let it being handled by the postProcessEntry function completely?

The main reason is readablity. The "natural" way of doing things is open -> do something -> close. The current way is do something -> close -> open new.

The function supposes that there is a transaction in progress (if any, obviously), and that there is "something" that will close the transaction afterwards.

From my point of view, either the function doesn't know a transaction is in progress (as it was before) or it knows and can handle transactions properly. If it knows, it shouldn't depend on others to open and / or close the transaction.

would it work if I simply add a comment "note: transaction is initiazed and closed elsewhere" to de-confuse devs ?

I don't feel it's worth spending time rewriting this bit if you balance the time needed (and retesting needed) with the small benefit of a bit less confusion. I'd rather add a comment then.

I agree that rewriting this bit could take some time to re-test, but I think it's something we should do now. I don't think that adding a comment is enough in this case.
Calling @butonic and / or @DeepDiver1975 to break the tie.

PVince81 · 2017-04-03T10:12:56Z

@jvillafanez I've adjusted the other bits

PVince81 · 2017-04-05T15:38:28Z

@butonic has tested this and confirmed that memory usage is reduced

mmattel · 2017-04-05T16:24:55Z

Great, do you have numbers that you can share, as we discussed above?

butonic · 2017-04-06T10:01:24Z

Tested with very long file and folder names:

[...]
	File   /u2/files/4 250 char dir name B9DB5C4FEAD4BF3BD562ED01816D3AC538568B981110E70E20565C5349F2D53C56B3947A39E8198D8757BBF9A3A3D827A06D4EC6BD418C48A898588D32B98BEC50BCF3D3E53B811DF921C632B9FEDEF9D8689C4DCC269DBF35B01D0F374B57CC6EC073586868475D6A47172B7DAAE41414C273/4 250 char dir name B9DB5C4FEAD4BF3BD562ED01816D3AC538568B981110E70E20565C5349F2D53C56B3947A39E8198D8757BBF9A3A3D827A06D4EC6BD418C48A898588D32B98BEC50BCF3D3E53B811DF921C632B9FEDEF9D8689C4DCC269DBF35B01D0F374B57CC6EC073586868475D6A47172B7DAAE41414C273/4 250 char dir name B9DB5C4FEAD4BF3BD562ED01816D3AC538568B981110E70E20565C5349F2D53C56B3947A39E8198D8757BBF9A3A3D827A06D4EC6BD418C48A898588D32B98BEC50BCF3D3E53B811DF921C632B9FEDEF9D8689C4DCC269DBF35B01D0F374B57CC6EC073586868475D6A47172B7DAAE41414C273/4 250 char dir name B9DB5C4FEAD4BF3BD562ED01816D3AC538568B981110E70E20565C5349F2D53C56B3947A39E8198D8757BBF9A3A3D827A06D4EC6BD418C48A898588D32B98BEC50BCF3D3E53B811DF921C632B9FEDEF9D8689C4DCC269DBF35B01D0F374B57CC6EC073586868475D6A47172B7DAAE41414C273/4 250 char dir name B9DB5C4FEAD4BF3BD562ED01816D3AC538568B981110E70E20565C5349F2D53C56B3947A39E8198D8757BBF9A3A3D827A06D4EC6BD418C48A898588D32B98BEC50BCF3D3E53B811DF921C632B9FEDEF9D8689C4DCC269DBF35B01D0F374B57CC6EC073586868475D6A47172B7DAAE41414C273/44444 250 char name B9DB5C4FEAD4BF3BD562ED01816D3AC538568B981110E70E20565C5349F2D53C56B3947A39E8198D8757BBF9A3A3D827A06D4EC6BD418C48A898588D32B98BEC50BCF3D3E53B811DF921C632B9FEDEF9D8689C4DCC269DBF35B01D0F374B57CC6EC073586868475D6A47172B7DAAE41414C273

+---------+--------+--------------+
| Folders | Files  | Elapsed time |
+---------+--------+--------------+
| 111115  | 100003 | 00:17:06     |
+---------+--------+--------------+

Memory profile without this PR looks like this:

mmattel · 2017-04-06T11:46:26Z

@butonic how is the profile looking with this PR?
The delta would be interesting

mmattel · 2017-04-06T12:45:02Z

Anyway, 👍

butonic · 2017-04-06T14:44:37Z

I did a test with with 9.1. I wrote a cachegrind when the first batch is commited:

My top stayed at:

  PID USER      PR  NI    VIRT    RES    SHR S  %CPU %MEM     ZEIT+ BEFEHL 
6369 www-data  20   0  374524  73952  19280 S  84,8  0,9   5:03.37 php occ files:scan u2 -v

25MB vs 196MB seems fine

mmattel · 2017-04-06T15:08:52Z

HURRA, congrats, pls merge !

@PVince81 I propose a note in the feature list of v10!

@settermjd I propose a documentation note which is helpful for physical servers sizing.
Something like Scanning of files is processed in 10k files chunks. Based on tests, server memory usage for scanning +10k files used about 25MB of memory. Due to the chunking algorithm, server memory usage will not grow.

PVince81 · 2017-04-06T15:29:01Z

Nah... need to first rearrange the code to make @jvillafanez and future devs who read this code happy

butonic · 2017-04-06T20:43:50Z

@mmattel 25Mb is only the amount of ram in use as seen by php-memprof. Top shows a usage of 73952kb so ~74Mb.

mmattel · 2017-04-18T14:22:13Z

Any update on this?

PVince81 · 2017-04-18T14:23:01Z

I don't have time to finish this, sorry

mmattel · 2017-04-18T14:48:59Z

would it work if I simply add a comment "note: transaction is initiazed and closed elsewhere" to de-confuse devs ?

Cant we merge with the suggested comment as note above and do the rest later?
The benefit of this PR is really big on low memory systems...

PVince81 · 2017-04-18T14:50:02Z

ok, here you go then

mmattel · 2017-04-18T14:51:19Z

Thanks, 😄

mmattel · 2017-04-18T15:09:51Z

Maybe a note at ownCloud 10.0 Features under General to be added ?
occ scanner optimized memory usage for large scans by using autocommits

PVince81 · 2017-04-18T15:11:24Z

@mmattel sounds good, can you add it ?

mmattel · 2017-04-18T15:23:50Z

ok will do, was not sure if I am allowed to.

mmattel · 2017-04-18T15:28:36Z

Done

PVince81 · 2017-05-18T07:20:11Z

stable9.1. #27881

lock · 2019-08-03T09:02:07Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

OCC scanner commit in batches

52769e1

PVince81 added the 3 - To Review label Mar 28, 2017

PVince81 added this to the 10.0 milestone Mar 28, 2017

PVince81 mentioned this pull request Mar 28, 2017

Huge transaction in CLI scanner #27516

Closed

jvillafanez reviewed Mar 29, 2017

View reviewed changes

Fix scanner after review

f354692

PVince81 merged commit 9ee487c into master Apr 18, 2017

PVince81 deleted the files-scan-commit-batch branch April 18, 2017 14:50

mmattel mentioned this pull request Apr 18, 2017

Adding a note at section System Requirements / Memory owncloud-archive/documentation#3003

Merged

tomneedham mentioned this pull request May 15, 2017

Backport file scanner commit changes in batches #27881

Merged

lock bot locked as resolved and limited conversation to collaborators Aug 3, 2019

OCC scanner commit in batches #27527

OCC scanner commit in batches #27527

Conversation

PVince81 commented Mar 28, 2017

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

mention-bot commented Mar 28, 2017

PVince81 commented Mar 28, 2017

mmattel commented Mar 28, 2017

mmattel commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Apr 3, 2017

PVince81 commented Apr 5, 2017

mmattel commented Apr 5, 2017

butonic commented Apr 6, 2017 • edited Loading

mmattel commented Apr 6, 2017

mmattel commented Apr 6, 2017

butonic commented Apr 6, 2017

mmattel commented Apr 6, 2017

PVince81 commented Apr 6, 2017

butonic commented Apr 6, 2017

mmattel commented Apr 18, 2017

PVince81 commented Apr 18, 2017

mmattel commented Apr 18, 2017

PVince81 commented Apr 18, 2017

mmattel commented Apr 18, 2017

mmattel commented Apr 18, 2017

PVince81 commented Apr 18, 2017

mmattel commented Apr 18, 2017

mmattel commented Apr 18, 2017

PVince81 commented May 18, 2017

lock bot commented Aug 3, 2019

butonic commented Apr 6, 2017 •

edited

Loading