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

OCC scanner commit in batches #27527

Merged
merged 2 commits into from Apr 18, 2017

Conversation

Projects
None yet
5 participants
@PVince81
Member

PVince81 commented Mar 28, 2017

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.

@PVince81 PVince81 added this to the 10.0 milestone Mar 28, 2017

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Mar 28, 2017

@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.

mention-bot commented Mar 28, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 28, 2017

Member

also @mmattel might be interested too

Member

PVince81 commented Mar 28, 2017

also @mmattel might be interested too

@mmattel

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Mar 28, 2017

Contributor

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.

Contributor

mmattel commented Mar 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Mar 28, 2017

Contributor

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

Contributor

mmattel commented Mar 28, 2017

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

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

This comment has been minimized.

@jvillafanez

jvillafanez Mar 29, 2017

Member

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

@jvillafanez

jvillafanez Mar 29, 2017

Member

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

This comment has been minimized.

@PVince81

PVince81 Apr 3, 2017

Member

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...

@PVince81

PVince81 Apr 3, 2017

Member

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...

This comment has been minimized.

@mmattel

mmattel Apr 3, 2017

Contributor

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

mmattel Apr 3, 2017

Contributor

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.

This comment has been minimized.

@PVince81

PVince81 Apr 3, 2017

Member

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

@PVince81

PVince81 Apr 3, 2017

Member

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

This comment has been minimized.

@mmattel

mmattel Apr 3, 2017

Contributor

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...

@mmattel

mmattel Apr 3, 2017

Contributor

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...

Show outdated Hide outdated lib/private/Files/Utils/Scanner.php
Show outdated Hide outdated 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) {

This comment has been minimized.

@jvillafanez

jvillafanez Mar 29, 2017

Member

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.

@jvillafanez

jvillafanez Mar 29, 2017

Member

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.

This comment has been minimized.

@PVince81

PVince81 Mar 29, 2017

Member

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.

@PVince81

PVince81 Mar 29, 2017

Member

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.

This comment has been minimized.

@jvillafanez

jvillafanez Mar 29, 2017

Member

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();
}
@jvillafanez

jvillafanez Mar 29, 2017

Member

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();
}

This comment has been minimized.

@PVince81

PVince81 Apr 3, 2017

Member

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.

@PVince81

PVince81 Apr 3, 2017

Member

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.

This comment has been minimized.

@jvillafanez

jvillafanez Apr 3, 2017

Member

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?

@jvillafanez

jvillafanez Apr 3, 2017

Member

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?

This comment has been minimized.

@PVince81

PVince81 Apr 3, 2017

Member

why?

@PVince81

This comment has been minimized.

@jvillafanez

jvillafanez Apr 3, 2017

Member

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.

@jvillafanez

jvillafanez Apr 3, 2017

Member

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.

This comment has been minimized.

@PVince81

PVince81 Apr 7, 2017

Member

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.

@PVince81

PVince81 Apr 7, 2017

Member

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.

This comment has been minimized.

@jvillafanez

jvillafanez Apr 7, 2017

Member

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.

@jvillafanez

jvillafanez Apr 7, 2017

Member

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

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 3, 2017

Member

@jvillafanez I've adjusted the other bits

Member

PVince81 commented Apr 3, 2017

@jvillafanez I've adjusted the other bits

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 5, 2017

Member

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

Member

PVince81 commented Apr 5, 2017

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

@mmattel

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Apr 5, 2017

Contributor

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

Contributor

mmattel commented Apr 5, 2017

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

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Apr 6, 2017

Member

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:

screenshot from 2017-04-05 13-54-06

Member

butonic commented Apr 6, 2017

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:

screenshot from 2017-04-05 13-54-06

@mmattel

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Apr 6, 2017

Contributor

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

Contributor

mmattel commented Apr 6, 2017

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

@mmattel

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Apr 6, 2017

Contributor

Anyway, 👍

Contributor

mmattel commented Apr 6, 2017

Anyway, 👍

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Apr 6, 2017

Member

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

screenshot from 2017-04-06 14-51-48

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

Member

butonic commented Apr 6, 2017

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

screenshot from 2017-04-06 14-51-48

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

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Apr 6, 2017

Contributor

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.

Contributor

mmattel commented Apr 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 6, 2017

Member

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

Member

PVince81 commented Apr 6, 2017

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

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Apr 6, 2017

Member

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

Member

butonic commented Apr 6, 2017

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

@mmattel

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Apr 18, 2017

Contributor

Any update on this?

Contributor

mmattel commented Apr 18, 2017

Any update on this?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 18, 2017

Member

I don't have time to finish this, sorry

Member

PVince81 commented Apr 18, 2017

I don't have time to finish this, sorry

@mmattel

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Apr 18, 2017

Contributor

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...

Contributor

mmattel commented Apr 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 18, 2017

Member

ok, here you go then

Member

PVince81 commented Apr 18, 2017

ok, here you go then

@PVince81 PVince81 merged commit 9ee487c into master Apr 18, 2017

5 checks passed

Scrutinizer 2 new issues, 1 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
license/cla Contributor License Agreement is signed.
Details

@PVince81 PVince81 deleted the files-scan-commit-batch branch Apr 18, 2017

@mmattel

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Apr 18, 2017

Contributor

Thanks, 😄

Contributor

mmattel commented Apr 18, 2017

Thanks, 😄

@mmattel

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Apr 18, 2017

Contributor

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

Contributor

mmattel commented Apr 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 18, 2017

Member

@mmattel sounds good, can you add it ?

Member

PVince81 commented Apr 18, 2017

@mmattel sounds good, can you add it ?

@mmattel

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Apr 18, 2017

Contributor

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

Contributor

mmattel commented Apr 18, 2017

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

@mmattel

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Apr 18, 2017

Contributor

Done

Contributor

mmattel commented Apr 18, 2017

Done

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 18, 2017

Member

stable9.1. #27881

Member

PVince81 commented May 18, 2017

stable9.1. #27881

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment