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.

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

Expire UserMap cache more aggressively and add cache debugging #5331

Merged
merged 4 commits into from
May 5, 2023

Conversation

pop4959
Copy link
Member

@pop4959 pop4959 commented May 5, 2023

Fix #5327

There are a couple of issues with the usercache, namely:

  • With a large / oversized heap, the maximum number of users that can be stored in the cache easily exceeds the number of total users. This generally means that the cache can and will store every single user on the server, which is usually not desirable (this defeats the purpose of using a cache).
  • In the linked issue, a 24 GB heap is being used, which means Essentials allows 25,769,803,776 / 1024 / 96 = 262,144 cache entries. The server in the issue only has ~ 63,000 users, but all 63,000 were being loaded resulting in almost 1 GB memory usage from Essentials which is unacceptable.
  • There is no expiry of values in the cache, even after not being used for a long time. This is a problem, as memory will start to leak anyways, especially when the maximum cache size is very large or effectively unbounded as mentioned above.

Solution:

  • More aggressively limit the maximum cache size. I'm not even sure if we want to use max memory size in the calculation here, but for lack of a better alternative that scales, I changed max memory / 1024 / 96 to max memory / 1024 / 1024.
  • Given the linked issue again, this would mean a maximum cache size of 24,576 < 63,000 meaning we will no longer ever store every single user. And that's with an oversized heap, for a normal server it will be far less and more reasonable. For a 1 GB heap the cache can store around 1000 users which is probably plenty for a server that size.
  • Expire values in the cache that are not being accessed. I've set the default to 10 minutes, as that seems plenty (if the user is not being accessed at all in 10 minutes they're probably not going to be accessed again for a while). We could be more aggressive with this, but I think this is good enough for now. It solves the main concern of values potentially never expiring, and shouldn't really negatively impact performance with supported usage.

Worth mentioning is that in the linked issue, for some reason, all ~63,000 users were being cached. This should not happen normally with typical usage of the cache, and implies that (probably some plugin) something is loading every user into the cache. To help debug this I've added another system property to log every time something gets added to the cache.

@pop4959 pop4959 requested a review from JRoy May 5, 2023 06:02
@pop4959 pop4959 added type: enhancement Features and feature requests. module: main Issues or PRs for the main Essentials module labels May 5, 2023
JRoy
JRoy previously approved these changes May 5, 2023
@JRoy JRoy merged commit 991bc61 into EssentialsX:2.x May 5, 2023
1 check passed
@pop4959 pop4959 deleted the cache-improvements branch May 5, 2023 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: main Issues or PRs for the main Essentials module type: enhancement Features and feature requests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UserMap uses a lot of memory with large number of users
2 participants