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

Remove the DefaultMaxDirectMemory configuration #51

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

postalservice14
Copy link

@postalservice14 postalservice14 commented Jan 3, 2024

This is in reference to this question/issue: #50

We've run into issue where switching to buildpacks caused issues. We found out that this calculator seemed to be arbitrarily setting the max direct memory to 10M, instead of letting the Java defaults happen.

Let the JVM set the default MaxDirectMemorySize, while still allowing it to be overridden by -XX:MaxDirectMemorySize

…ts default, while still allowing it to be overridden by -XX:MaxDirectMemorySize
Copy link

linux-foundation-easycla bot commented Jan 3, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: postalservice14 / name: John Kelly (c9ed5f5)

@dmikusa
Copy link
Contributor

dmikusa commented Jan 4, 2024

I can't say for sure why the default is set to 10M, that happened before my time. What I can say is that I know past buildpack maintainers usually made decisions like this based on a.) based on feedback from users and b.) load-testing experiments.

If I had to guess, I'd bet the rationale is probably related to either a.) the JVM allocating more native memory than was typically necessary by default or b.) the JVM doing something that is dynamic by default and thus potentially causing container OOME problems. The buildpack has to be very meticulous to ensure that the JVM does not exceed the container memory limit and a big part of that is the memory calculator fixing memory regions.

I'm not saying that these values should last for all of time and never be changed, but I think we'd need to research the topic to understand the JVM behavior past and present (i.e. how it works from Java 8 to 21), as well as what common servers and frameworks (like Netty and Spring) are requiring for apps in terms of Direct Memory.

The reason for being so careful here is that this is the default setting. Making a change to a default can be potentially breaking for many users. Users likely have containers sized with the expectation the JVM will consume 10M of Direct Memory, but after this change, the JVM may take more than that by default. That could result in unexpected container OOMEs and users needing to resize apps.

If you want to change the default value for your users, you can set an Operator Default that overrides the direct memory setting. I don't think that will let you null out the value and allow the JVM to calculate the value, but it will let you default to a larger setting if you have many apps that need more default memory.

I suppose we could look at a way to tell the memory calculator to not set the direct memory, possibly a new config setting, and thus let the JVM take over. However, we'd still want to do some research and make sure this isn't going to cause potential OOME container memory issues.

My $0.02. @anthonydahanne and @pivotal-david-osullivan feel free to chime in as well.

@postalservice14
Copy link
Author

Thanks @dmikusa - I actually incorrectly thought this was the memory calculator that was used in our buildpack setup, when in reality the paketo-buildpacks/libjvm is actually where it's happening. I'll leave this PR open in case you want it, but we can continue the discussion over here if you'd like: https://github.com/orgs/paketo-buildpacks/discussions/241

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants