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

[GR-52129] Extend javadoc and set driver memory limit. #8384

Merged
merged 3 commits into from
Feb 20, 2024

Conversation

graalvmbot
Copy link
Collaborator

@graalvmbot graalvmbot commented Feb 15, 2024

This integrates #8230 and addresses #8366.

zakkak and others added 2 commits January 26, 2024 14:26
Document that using `""` as the package name results in all packages and
classes being registered for build time initialization.
The change only applies to the bash launcher and makes sure it uses a similar limit that is also used in the native version of the driver.
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Feb 15, 2024
Copy link
Collaborator

@zakkak zakkak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @fniephaus

Copy link
Collaborator

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

driver_exe_build_args = driver_build_args + svm_experimental_options([
'-H:+AllowJRTFileSystem',
'-H:IncludeResources=com/oracle/svm/driver/launcher/.*',
'-H:-ParseRuntimeOptions',
f'-R:MaxHeapSize={256 * 1024 * 1024}',
f'-R:MaxHeapSize={driver_max_memory_in_mb * 1024 * 1024}',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inconsistent. When the driver runs on JVM we give it 80% of 256MB but when it runs as image we give it 256MB MaxHeapSize, which is not the same. Either compute 80% of 256MB in mx_substratevm.py and use MaxHeapSize in both cases or multiply 0.8 here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jerboaa would you be ok with using MaxHeapSize in both cases? I'm not sure your suggestion in here is actually correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure your suggestion in #8366 (comment) is actually correct.

It's certainly more complex than just using MaxHeapSize of 80% of 256MB for both the jvm case and the native image case. You can even share the same arg-string but prefix with -R: in the native image case and -XX: in the jvm case.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jerboaa would you be ok with using MaxHeapSize in both cases?

It makes more sense to constrain the entire JVM to work within the set limit (MaxRAM), and not just limit the amount of heap space. So I'd prefer the MaxRAM approach. Your mileage may vary.

I'm not sure your suggestion in here is actually correct.

Why wouldn't it? Could you explain?

$ java -XX:MaxRAM=256m -XX:MaxRAMPercentage=80.0 -XX:+PrintFlagsFinal --version | grep -E 'MaxHeap|MaxRAM'
    uintx MaxHeapFreeRatio                         = 70                                     {manageable} {default}
   size_t MaxHeapSize                              = 216006656                                 {product} {ergonomic}
 uint64_t MaxRAM                                   = 268435456                              {pd product} {command line}
    uintx MaxRAMFraction                           = 4                                         {product} {default}
   double MaxRAMPercentage                         = 80.000000                                 {product} {command line}
   size_t SoftMaxHeapSize                          = 216006656                              {manageable} {ergonomic}
$ java -XX:MaxRAM=335544320 -XX:MaxRAMPercentage=80.0 -XX:+PrintFlagsFinal --version | grep -E 'MaxHeap|MaxRAM'
    uintx MaxHeapFreeRatio                         = 70                                     {manageable} {default}
   size_t MaxHeapSize                              = 268435456                                 {product} {ergonomic}
 uint64_t MaxRAM                                   = 335544320                              {pd product} {command line}
    uintx MaxRAMFraction                           = 4                                         {product} {default}
   double MaxRAMPercentage                         = 80.000000                                 {product} {command line}
   size_t SoftMaxHeapSize                          = 268435456                              {manageable} {ergonomic}

Copy link
Member

@olpaw olpaw Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'd prefer the MaxRAM approach.

But we don't have the MaxRAMPercentage GC option for the image mode so I rather have the same option (MaxHeapSize aka Xmx) used in both forms of execution.

Also I do not see any advantage in using -XX:MaxRAM=256m because whenever actual physical ram would be less than 256m (which is the only case where final MaxHeapSize would diverge from "80% of 256m") you would certainly not be able to run the image builder JVM at all (even if the driver would be fine with that memory constraints).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I do not see any advantage in using -XX:MaxRAM=256m because whenever actual physical ram would be less than 256m (which is the only case where final MaxHeapSize would diverge from "80% of 256m") you would certainly not be able to run the image builder JVM at all (even if the driver would be fine with that memory constraints).

It sounds like we are still not on the same page as to what MaxRAM does. I'm arguing that there is value for the (JVM) process to get both MaxRAM and some heap size setting (be it MaxHeapSize or MaxRAMPercentage).

The point of using MaxRAM is to tell the launcher JVM to stay within that (for all of its operations not just the heap). It's the delta d = A - MaxHeapSize, which wouldn't have an upper bound otherwise. Where A is the "detected" physical memory. Setting MaxRAM makes A = MaxRAM (always) and therefore, d <= MaxRAM - MaxHeapSize. That is, by setting MaxRAM and MaxHeapSize you know what the delta can be in the worst case. That helps if you need to reason about where the memory goes when running the image generator which usually forks two JVMs. The launcher process and the image generator. Say, you'd run the image generator in a container with 5GB of memory, you can then reason that the launcher jvm stays within the MaxRAM setting, say 256mb, so the native image generator would have at least 5GB - 256mb of memory to use. You cannot reason similarly by only restricting heap size of the JVM as you don't know what d will be for the launcher process (worst case).

Anyway, I can accept that you think it's OK to only set heap size.

@@ -1054,7 +1054,9 @@ def _native_image_launcher_extra_jvm_args():
Gets the extra JVM args needed for running com.oracle.svm.driver.NativeImage.
"""
# Support for running as Java module
res = []
res = [
f'-XX:{max_heap_size_flag}', '-XX:MaxRAMPercentage=80',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you set -XX:MaxHeapSize= then MaxRAMPercentage isn't being used, so it makes no sense to set them both. MaxRAMPercentage only comes into play if no values for the heap size are being given (in which case the JVM uses a percentage of physical memory). It makes sense to set it when MaxRAM is being set - which overrides physical memory detection -, since it then uses the set percentage for the heap size as no explicit heap size setting is been done otherwise.

@graalvmbot graalvmbot force-pushed the fniephaus/GR-52129/docs-and-extra-flags branch from ff471bb to 91645c6 Compare February 19, 2024 17:11
@graalvmbot graalvmbot closed this Feb 20, 2024
@graalvmbot graalvmbot merged commit 60833b2 into master Feb 20, 2024
12 checks passed
@graalvmbot graalvmbot deleted the fniephaus/GR-52129/docs-and-extra-flags branch February 20, 2024 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants