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.

Sign up for GitHub

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

Jump to bottom

Improving memory usage / cache usage #24403

Closed
PVince81 opened this issue May 3, 2016 · 18 comments
Closed

Improving memory usage / cache usage #24403

PVince81 opened this issue May 3, 2016 · 18 comments
Assignees
@PVince81
Labels
performance sev2-high technical debt
Milestone
10.0

Comments

@PVince81
Copy link
Contributor

PVince81 commented May 3, 2016

From @butonic:

A non exhaustive search reveals other places where an array is used to cache data:

apps/user_ldap/group_ldap.php#L46
apps/user_ldap/group_ldap.php#L51
apps/dav/lib/connector/sabre/sharesplugin.php#L67
apps/dav/lib/connector/sabre/tagsplugin.php#L80
apps/dav/lib/connector/sabre/custompropertiesbackend.php#L75
lib/private/Encryption/File.php#L35
lib/private/Files/Config/UserMountCache.php#L54
lib/private/Files/Config/UserMountCache.php#L61
lib/private/Files/Storage/LocalTempFileTrait.php#L40

We should check them after we decide how to handle this. Not all of them justify using a CappedMemoryCache, eg: lib/private/Config.php#L42

@DeepDiver1975 @rullzer @icewind1991 @schiesbn @nickvergessen
@PVince81 PVince81 added this to the backlog milestone May 3, 2016
@PVince81
Copy link
Contributor Author

PVince81 commented May 3, 2016

More context: when running background jobs, many classes that are used might cache objects forever. This causes the memory usage to increase significantly and many cached objects aren't necessarily reused.

So one idea was to introduce the CappedCache everywhere an array is used for caching.

@rullzer
Copy link
Contributor

rullzer commented May 3, 2016

Using a capped cache is a quickfix. But we should evaluate each change as to not break stuff when the cache is expected to hold more elements...

@nickvergessen
Copy link
Contributor

I'd also say that it is just hiding other issues, but yeah, maybe we can set the size to something really huge for debug mode, so it still wastes memory for developers?

@PVince81
Copy link
Contributor Author

PVince81 commented May 9, 2016 •
edited
Loading

From @butonic in the other ticket, itemized:

@PVince81
Copy link
Contributor Author

PVince81 commented May 9, 2016 •
edited
Loading

  • "$app->generateBirthdays();"

@PVince81
Copy link
Contributor Author

PVince81 commented May 9, 2016

@blizzz can you do the LDAP one, @icewind1991 the mount caches and FS ones, @DeepDiver1975 generateBirthdays ?

The Sabre ones are low prio because they aren't called during background jobs or updates.

@nickvergessen
Copy link
Contributor

generateBirthdays is a meta one, it calls the userManager, so nothing to do there.

@PVince81
Copy link
Contributor Author

PVince81 commented May 9, 2016

@MorrisJobke MorrisJobke modified the milestones: 9.1-current, backlog May 12, 2016
@butonic
Copy link
Member

butonic commented May 12, 2016

This breaks upgrades on instances with more than ~10000-15000 users

@felixboehm
Copy link
Contributor

@cmonteroluque This needs backport for 9.0.3

@PVince81
Copy link
Contributor Author

Not sure if we can backport ALL of them once they are fixed, but we should at least backport the most impactful pieces.

@ghost
Copy link

ghost commented May 19, 2016

@PVince81 @felixboehm we should talk about which get backported. Later today during the issue review call

rullzer added a commit that referenced this issue May 27, 2016
@rullzer
Use a capped memory cache for the user/group cache
259906c 
For #24403
When upgrading huge installations this can lead to memory problems as
the cache will only grow and grow.

Capping this memory will make sure we don't run out while during normal
operation still basically cache everything.
@rullzer rullzer mentioned this issue May 27, 2016
Merged
@rullzer
Copy link
Contributor

rullzer commented May 27, 2016

apps/user_ldap/group_ldap.php#L46
apps/user_ldap/group_ldap.php#L51

PR in #24869

DeepDiver1975 pushed a commit that referenced this issue May 30, 2016
@rullzer @DeepDiver1975
Use a capped memory cache for the user/group cache
dc4fd43 
For #24403
When upgrading huge installations this can lead to memory problems as
the cache will only grow and grow.

Capping this memory will make sure we don't run out while during normal
operation still basically cache everything.
rullzer added a commit that referenced this issue Jun 1, 2016
@rullzer
Use a capped memory cache for the user/group cache
0efe3ba 
For #24403
When upgrading huge installations this can lead to memory problems as
the cache will only grow and grow.

Capping this memory will make sure we don't run out while during normal
operation still basically cache everything.
@rullzer rullzer mentioned this issue Jun 1, 2016
Merged
rullzer added a commit that referenced this issue Jun 1, 2016
@rullzer
Use a capped memory cache for the user/group cache
f582521 
For #24403
When upgrading huge installations this can lead to memory problems as
the cache will only grow and grow.

Capping this memory will make sure we don't run out while during normal
operation still basically cache everything.
@rullzer rullzer mentioned this issue Jun 1, 2016
Merged
@PVince81 PVince81 self-assigned this Jun 10, 2016
@PVince81 PVince81 mentioned this issue Jun 10, 2016
Merged
@PVince81
Copy link
Contributor Author

PR for user mount cache: #25056

PR for encryption: #25055

@PVince81 PVince81 modified the milestones: 9.1-current, 9.2-next Jun 15, 2016
@PVince81
Copy link
Contributor Author

Oops, this should stay on 9.1.

Once the backports are merged, the other remaining cases are not really relevant to memory usage. For example Sabre plugin aren't used when running cron operations or updates.

@PVince81 PVince81 modified the milestones: 9.1-current, 9.2-next Jun 15, 2016
@PVince81
Copy link
Contributor Author

Regarding lib/private/Files/Storage/LocalTempFileTrait.php#L40 I don't think we can make this a capped cache as we'd lose track of what temp files were created and will be unable to clean them up afterwards.
Also I think these kind of temp files are only used when uploading/downloading files from external storage, so shouldn't be triggered as part of cron or updates.

Capped cache for Encryption cache info cache and UserMountCache have been backported to stable9.

The Sabre ones aren't relevant for cron or updates.

Moving the rest to 9.2 now.

@butonic let me know if you think otherwise

@PVince81 PVince81 modified the milestones: 9.2-next, 9.1-current Jun 16, 2016
This was referenced Jun 17, 2016
Merged
Merged
@PVince81
Copy link
Contributor Author

I think we're done here, see my comment for the unticked cases.
Closing.

@lock
Copy link

lock bot commented Aug 3, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@PVince81 PVince81

Labels
performance sev2-high technical debt
Projects
None yet
Milestone
10.0
Development

No branches or pull requests
6 participants
@rullzer @nickvergessen @MorrisJobke @PVince81 @butonic @felixboehm