Skip to content

Conversation

@rkennke
Copy link
Contributor

@rkennke rkennke commented Mar 27, 2019

Currently, -Xms is overloaded to also be the threshold for shrinking the heap. This seems counter-intuitive, esp. when coming from JDK. This change proposes to introduce a separate option, very similar to the same option in JDK. This should be less surprising.

@rkennke
Copy link
Contributor Author

rkennke commented Mar 27, 2019

Apparently this is violating NO_ALLOCATION restriction somehow. I don't fully understand through which call path that happens, and how to avoid it. Will dig a little deeper tomorrow. Any hints are appreciated.

@rkennke
Copy link
Contributor Author

rkennke commented Mar 29, 2019

Looks like a beginners mistake: I used + to concat strings. Should be fixed.

@graalvmbot
Copy link
Collaborator

Hello Roman Kennke, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address rkennke -(at)- linux -(dot)- fritz -(dot)- box. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

@rkennke
Copy link
Contributor Author

rkennke commented Mar 29, 2019

Oops, sorry, I accidentally pushed from a box that had git not properly set-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can avoid the format issue by appending // to the end of the annotation line, like the others do.

Copy link
Member

Choose a reason for hiding this comment

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

Or surround the options with // @formatter:off and // @formatter:on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Very good to know! Should be good now.

@rkennke
Copy link
Contributor Author

rkennke commented Apr 1, 2019

The remaining CI failure doesn't look like related to my change.

@dougxc
Copy link
Member

dougxc commented Apr 3, 2019

This PR should not include the substratevm/native-image binary file.

@rkennke
Copy link
Contributor Author

rkennke commented Apr 3, 2019

Oops. That slipped in by accident. Should be removed now. Thanks for pointing out!

@rkennke rkennke closed this May 22, 2019
@rkennke rkennke deleted the svm-maxfreeheapsize branch May 22, 2019 10:29
@rkennke rkennke restored the svm-maxfreeheapsize branch May 22, 2019 10:31
@rkennke
Copy link
Contributor Author

rkennke commented May 22, 2019

Accidentally closed the branch. Reopening...

@rkennke rkennke reopened this May 22, 2019
@rkennke
Copy link
Contributor Author

rkennke commented May 22, 2019

I merged in latest changes from master, and all checks passed. From here, the PR is ready to go.

}
}

@Fold
Copy link

@christianwimmer christianwimmer May 24, 2019

Choose a reason for hiding this comment

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

This @Fold does not make much sense. It means that the option value of a runtime option is read and then constant folded during image generation.

You can either declare MaxHeapFreeRatio as a HostedOptionKey - then you can specify the option only during image generation and it is automatically constant folded even without the @Fold annotation.

Or you can remove the @Fold and get a "real" option that you can set at run time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, Christias! I removed the Fold. Remaining test failure doesn't look related.

/* If I am under the minimum heap size, then I can keep this chunk. */
final boolean result = bytesInUse.belowThan(minimumHeapSize);
/* If I am under the max heap free ratio, then I can keep this chunk. */
final boolean result = bytesInUse.multiply(100).unsignedDivide(maximumHeapSize).aboveThan(100 - HeapPolicy.getMaxHeapFreeRatio());

Choose a reason for hiding this comment

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

the multiplication with 100 is prone to integer overflows: on 32-bit architectures (where a word is a 32-bit value) the multiplication will overflow with many practical heap sizes.

You can easily reorder the arithmetic to be overflow free: bytesInUse < (maximumHeapSize / 100) * maxHeapFreeRatio

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the formula to be overflow-free (slightly differently to what you suggested to preserve the semantics), should be good now. Thanks for pointing out.

public static final HostedOptionKey<Long> LargeArrayThreshold = new HostedOptionKey<>(HeapPolicy.LARGE_ARRAY_THRESHOLD_SENTINEL_VALUE);

@Option(help = "The maximum percentage of heap free after GC to avoid shrinking") //
public static final RuntimeOptionKey<Integer> MaxHeapFreeRatio = new RuntimeOptionKey<>(70);

Choose a reason for hiding this comment

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

70% sound like a very high value. Considering that the default maximum heap size is also a large fraction of the physical memory size, this means that we are basically never returning chunks back to the operating system.

What workloads did you use to evaluate the change? How does it affect performance vs. memory footprint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

70% comes from the default of the same value in JDK. It seems a bit confusing compared to the previous logic: before we kept chunks when used < minheap, now we keep chunks when free < MaxHeapFree. Which means that with a value of 70%, we keep chunks when used is > 30%. Does that still sound bad?

@rkennke
Copy link
Contributor Author

rkennke commented May 5, 2021

I'm closing this due to lack of interest on my side.

@rkennke rkennke closed this May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants