CMS: wrong max_eden_size for check_gc_overhead_limit

[Volker Simonis]

Hi Jon,

you're right, Ivan is covered by the SAP contributor agreement.

I've opened "8135318: CMS: wrong max_eden_size for
check_gc_overhead_limit"
(https://bugs.openjdk.java.net/browse/JDK-8135318) for the issue.

Ivan will follow up on the question if this also affects other GCs.

Thanks for sponsoring the fix,
Volker


On Wed, Sep 9, 2015 at 6:22 PM, Jon Masamitsu <jon.masamitsu at oracle.com> wrote:
> 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
>
>