CMS: wrong max_eden_size for check_gc_overhead_limit

[Galkin, Ivan]

Hello Jon,

thank you for your review and your support.

Besides CMS the function check_gc_overhead_limit() is called only in PSMarkSweep, PSParallelCompact and PSScavenge:

*  PSMarkSweep, PSParallelCompact use the formula "young_gen->max_size() - young_gen->from_space()->capacity_in_bytes() - young_gen->to_space()->capacity_in_bytes()", which is correct because PSYoungGen::max_size() == "size in bytes reserved for young generation"

*  PSScavenge use a similar calculation, however both max_young_size and survivor_limit are recalculated because of MinHeapFreeRation & MaxHeapFreeRatio

// START SNIPPET FROM PSScavenge::invoke_no_policy(), see hotspot/src/share/vm/gc/parallel/psScavenge.cpp
        size_t max_young_size = young_gen->max_size();
        ...
        if (MinHeapFreeRatio != 0 || MaxHeapFreeRatio != 100) {
          max_young_size = MIN2(old_gen->capacity_in_bytes() / NewRatio, young_gen->max_size());
        }
        ...
        size_t survivor_limit =
          size_policy->max_survivor_size(max_young_size);
        ...
          assert(young_gen->max_size() >
            young_gen->from_space()->capacity_in_bytes() +
            young_gen->to_space()->capacity_in_bytes(),
            "Sizes of space in young gen are out-of-bounds");
        ...
          size_t max_eden_size = max_young_size -
            young_gen->from_space()->capacity_in_bytes() -
            young_gen->to_space()->capacity_in_bytes();
// END SNIPPET FROM PSScavenge::invoke_no_policy()

I'm not ably to say if there is an error in the calculation of max_eden_size, however I'm wondering a) why recalculated survivor_limit is not used and b) what is the marked assertion good for?
In my opinion the calculation should look as following. I didn't test the change. It's only deduced, so every comment would be welcome.

diff -r f74b3ce62e1f src/share/vm/gc/parallel/psScavenge.cpp
--- a/src/share/vm/gc/parallel/psScavenge.cpp Fri Sep 04 17:33:56 2015 -0700
+++ b/src/share/vm/gc/parallel/psScavenge.cpp             Thu Sep 10 12:30:19 2015 +0200
@@ -560,18 +560,14 @@
         if (UseAdaptiveGenerationSizePolicyAtMinorCollection &&
             (AdaptiveSizePolicy::should_update_eden_stats(gc_cause))) {
           // Calculate optimal free space amounts
-          assert(young_gen->max_size() >
-            young_gen->from_space()->capacity_in_bytes() +
-            young_gen->to_space()->capacity_in_bytes(),
+          assert(max_young_size > 2 * survivor_limit,
             "Sizes of space in young gen are out-of-bounds");
           size_t young_live = young_gen->used_in_bytes();
           size_t eden_live = young_gen->eden_space()->used_in_bytes();
           size_t cur_eden = young_gen->eden_space()->capacity_in_bytes();
           size_t max_old_gen_size = old_gen->max_gen_size();
-          size_t max_eden_size = max_young_size -
-            young_gen->from_space()->capacity_in_bytes() -
-            young_gen->to_space()->capacity_in_bytes();
+          size_t max_eden_size = max_young_size - 2 * survivor_limit;
           // Used for diagnostics
           size_policy->clear_generation_free_space_flags();

Best regard,
Ivan

P.S. Your questions about contributor agreement and a bug report were answered by my colleague Volker Simonis


From: hotspot-gc-dev [mailto:hotspot-gc-dev-bounces at openjdk.java.net] On Behalf Of Jon Masamitsu
Sent: Mittwoch, 9. September 2015 18:22
To: hotspot-gc-dev at openjdk.java.net
Subject: Re: CMS: wrong max_eden_size for check_gc_overhead_limit

Ivan,

You're correct about this bug.

Is it correct that you're covered by the SAP contributor
agreement?

Have you filed a bug report on this?

Have you checked the other GC's for this problem?

I can sponsor this fix.

Thanks.

Jon


On 9/8/2015 2:21 AM, Galkin, Ivan wrote:
Hello all,

I believe the calculation of max_eden_size which is needed to check the GC overhead in CMS is incorrect. Namely in concurrentMarkSweepGeneration.cpp the following expression is used:

size_t max_eden_size = _young_gen->max_capacity() - _young_gen->to()->capacity() - _young_gen->from()->capacity();

According to the implementation of DefNewGeneration::max_capacity() the expression can be unfolded as:

                size_t max_eden_size = ("reserved size of young gen" - "size of survivor space") - "size of survivor space" - "size of survivor space";

So the value becomes too small (survival spaces are accounted 3 times!), which can lead to the following problems:


1.       max_eden_size is too small and GC overhead is wrongfully recognized too early

2.       max_eden_size == 0 (all young spaces have the same size; -XX:SurvivorRatio=1) and GC overhead is never happens (see implementation of AdaptiveSizePolicy::check_gc_overhead_limit:

a.       max_eden_size == 0 leads to mem_free_eden_limit == 0;

b.      free_in_eden < mem_free_eden_limit is always false, since both are unsigned integers and mem_free_eden_limit is 0)

I would therefore suggest the following fix (DefNewGeneration:: max_eden_size() already contains the correctly calculated capacity of eden):

diff -r f74b3ce62e1f src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp
--- a/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp         Fri Sep 04 17:33:56 2015 -0700
+++ b/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp      Mon Sep 07 18:08:39 2015 +0200
@@ -1563,9 +1563,7 @@
   do_compaction_work(clear_all_soft_refs);

   // Has the GC time limit been exceeded?
-  size_t max_eden_size = _young_gen->max_capacity() -
-                         _young_gen->to()->capacity() -
-                         _young_gen->from()->capacity();
+  size_t max_eden_size = _young_gen->max_eden_size();
   GCCause::Cause gc_cause = gch->gc_cause();
   size_policy()->check_gc_overhead_limit(_young_gen->used(),
                                          _young_gen->eden()->used(),

Could anybody please review and sponsor the change?

Thank you in advance,
Ivan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150910/69c32f52/attachment.htm>