Better callForAllUsers memory usage #24327

Merged
merged 2 commits into from May 2, 2016

Projects

None yet

7 participants

@nickvergessen
Contributor

Should help to reduce the memory usage when callForAllUsers() is called https://github.com/owncloud/enterprise/issues/1298

@DeepDiver1975 @butonic

nickvergessen added some commits Apr 28, 2016
@nickvergessen nickvergessen Don't loop over the backends, we already know where the user should be 76af701
@nickvergessen nickvergessen When we iterate over all users that might be too much memory
b8114bc
@nickvergessen nickvergessen added this to the 9.1-current milestone Apr 28, 2016
@mention-bot

By analyzing the blame information on this pull request, we identified @icewind1991, @DeepDiver1975 and @blizzz to be potential reviewers

@nickvergessen nickvergessen changed the title from Better call for all users performance to Better callForAllUsers memory usage Apr 28, 2016
@DeepDiver1975 DeepDiver1975 commented on the diff Apr 28, 2016
lib/private/user/manager.php
continue;
}
+ $user = $this->getUserObject($uid, $backend, false);
$return = $callback($user);
@DeepDiver1975
DeepDiver1975 Apr 28, 2016 Member

I'd say there is a unset($user) missing ...
... assuming we iterate once over all users (500k+ e.g.) - we only need each user once - but we should free the memory after calling the callback

@icewind1991
icewind1991 Apr 28, 2016 Member

php should gc the old $user instance when the variable is re-asigned

@DeepDiver1975
DeepDiver1975 Apr 28, 2016 Member

yeah ... should 🙈

@DeepDiver1975
DeepDiver1975 Apr 28, 2016 Member

is there a way to force gc on php?

I know from other systems that gc cycles need some idle time to be invoked - no idea if this applies to php as well

@icewind1991
icewind1991 Apr 28, 2016 Member

Not related to this pr but useful knowledge for any php memory manipulations:

The most reliable way I have found to have php cleanup it's memory is to have a variable go out of scope (i.e. move the memory consuming task to a new function so all newly allocated objects go out of scope the moment the task is done)
Re-assigning a variable or unsetting it all seem to only work if the gc feels like it

@butonic
butonic Apr 28, 2016 Member

@DeepDiver1975 an unset($user) here wont help. I think @nickvergessen is on the spot - the problem is $this->cachedUsers[$uid] = new User($uid, $backend, $this, $this->config); I like @icewind1991 s proposal to use a CappedMemoryCache.

For what it is worth, gc_collect_cycles() should run the garbage collector. It won't free the $this->cachedUsers[$uid] however.

@DeepDiver1975
DeepDiver1975 Apr 28, 2016 Member

For what it is worth, gc_collect_cycles() should run the garbage collector. It won't free the $this->cachedUsers[$uid] however.

sure - those are all cached and as a result will no gc'd 🙈

@icewind1991
Member
icewind1991 commented Apr 28, 2016 edited

A better approach might be to replace the cachedUsers array with a CappedMemoryCache

that way it also fixes the memory usage if any other situation where create query a lot of user objects

@nickvergessen
Contributor

A better approach might be to replace the cachedUsers array with a CappedMemoryCache
that way it also fixes the memory usage if any other situation where create query a lot of user objects

@PVince81 suggested the same at lunch, but I think in general it's good to know where things like that happen and callForAllUsers should only happen on install/update/background job anyway, so this is my prefered way.

@DeepDiver1975
Member

and callForAllUsers should only happen on install/update/background job anyway,

that's the current use case - but for sure not limited to this ...

@nickvergessen
Contributor

So do we want to switch to cappedcache now, or use this for backport and use the cappedcache only in 9.1+ ?

@nickvergessen
Contributor
@DeepDiver1975
Member

Yes - let's move this in 👍

@DeepDiver1975
Member

backport request added @karlitschek

@karlitschek
Member

nice. makes total sense. please backport 👍

@DeepDiver1975
Member

nice. makes total sense. please backport 👍

@nickvergessen can I ask you to submit the pr? THX

@nickvergessen
Contributor

Backport in #24384

Not sure if we have to (should) force this into 9.0.2 so shortly

@DeepDiver1975 DeepDiver1975 merged commit d7eb17b into master May 2, 2016

22 of 23 checks passed

smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_basicSync@0,mirallBranch=v2.0.2,slave=SMASH Build #14182 failed in 1 hr 7 min
Details
Scrutinizer No new issues
Details
cla-bot-core Build #3642 succeeded in 1 min 45 sec
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
core-ci-linux-jsunit/database=sqlite,label=SLAVE Build #61444 succeeded in 1 min 21 sec
Details
core-ci-linux-swift-primary-storage/database=mysql,label=SLAVE Build #55626 succeeded in 8 min 31 sec
Details
core-ci-linux/database=mysql,label=SLAVE Build #30084 succeeded in 10 min
Details
core-ci-linux/database=oci,label=SLAVE Build #30084 succeeded in 12 min
Details
core-ci-linux/database=pgsql,label=SLAVE Build #30084 succeeded in 9 min 48 sec
Details
core-ci-linux/database=sqlite,label=SLAVE Build #30084 succeeded in 5 min 7 sec
Details
ocs-api-integration-tests-ci Build #10256 succeeded in 8 min 25 sec
Details
server-master-linux-externals-ci/database=sqlite,external=smb-silvershell,label=SLAVE Build #9865 succeeded in 1 min 22 sec
Details
server-master-linux-externals-ci/database=sqlite,external=swift-ceph,label=SLAVE Build #9865 succeeded in 2 min 28 sec
Details
server-master-linux-externals-ci/database=sqlite,external=webdav-ownCloud,label=SLAVE Build #9865 succeeded in 2 min 51 sec
Details
server-master-linux-externals-smb-windows-ext-ci/database=sqlite,external=smb-windows,label=master Build #13315 succeeded in 2 min 21 sec
Details
server-master-linux-php5.4-ci/database=sqlite,label=SLAVE Build #3030 succeeded in 3 min 46 sec
Details
server-master-linux-php7-ci/database=sqlite,label=SLAVE Build #38311 succeeded in 2 min 31 sec
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=litmus,mirallBranch=v2.0.2,slave=SMASH Build #14182 succeeded in 3 min 15 sec
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_basicSync@1,mirallBranch=v2.0.2,slave=SMASH Build #14182 succeeded in 1 hr 22 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_shareLink,mirallBranch=v2.0.2,slave=SMASH Build #14182 succeeded in 1 hr 29 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePermissions,mirallBranch=v2.0.2,slave=SMASH Build #14182 succeeded in 1 hr 19 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePropagationGroups,mirallBranch=v2.0.2,slave=SMASH Build #14182 succeeded in 5 min 16 sec
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePropagationInsideGroups,mirallBranch=v2.0.2,slave=SMASH Build #14182 succeeded in 7 min 26 sec
Details
@DeepDiver1975 DeepDiver1975 deleted the better-callForAllUsers-performance branch May 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment