RFR (round 3), JEP-318: Epsilon GC

[Per Liden]

Hi Aleksey,

On 05/21/2018 03:07 PM, Aleksey Shipilev wrote:
> Hi,
> 
> This is third (and hopefully final) round of code review for Epsilon GC changes.
> 
> JEP, targeted to 11:
>    http://openjdk.java.net/jeps/318
>    (you can find links to binary builds and sandbox locations there)
> 
> Webrev:
>    http://cr.openjdk.java.net/~shade/epsilon/webrev.07/

Sorry for not reviewing sooner. A few minor comments:


make/autoconf/hotspot.m4
------------------------

  399   NON_MINIMAL_FEATURES="$NON_MINIMAL_FEATURES cmsgc g1gc 
parallelgc serialgc epsilongc jni-check jvmti management nmt services 
vm-structs"

There seems to be an emerging convention that experimental VM features 
should not be built by default. I.e. it shouldn't be part of 
NON_MINIMAL_FEATURES, instead the builder should use 
--with-jvm-features=epsilongc to enable it. Jesper is working on an 
informational JEP to clarify this, but I haven't seen it yet.

(Btw, ZGC, Shenandoah, etc will also be subject to this).


src/hotspot/share/gc/epsilon/epsilonArguments.cpp
-------------------------------------------------

   48 #if INCLUDE_EPSILONGC
   49   if (EpsilonMaxTLABSize < MinTLABSize) {
   50     warning("EpsilonMaxTLABSize < MinTLABSize, adjusting it to " 
SIZE_FORMAT, MinTLABSize);
   51     EpsilonMaxTLABSize = MinTLABSize;
   52   }
   53
   54   if (!EpsilonElasticTLAB && EpsilonElasticTLABDecay) {
   55     warning("Disabling EpsilonElasticTLABDecay because 
EpsilonElasticTLAB is disabled");
   56     FLAG_SET_DEFAULT(EpsilonElasticTLABDecay, false);
   57   }
   58 #endif

This section shouldn't need #if INCLUDE_EPSILONGC


src/hotspot/share/gc/epsilon/epsilonHeap.hpp
--------------------------------------------

  109   virtual bool supports_tlab_allocation()           const { return 
UseTLAB;        }

I think you just want to return true here, unless Epsilon has some 
special handling of UseTLAB that I'm missing.


src/hotspot/share/gc/epsilon/epsilonMemoryPool.hpp
--------------------------------------------------

   28 #if INCLUDE_EPSILONGC
   29 #include "gc/epsilon/epsilonHeap.hpp"
   30 #include "services/memoryPool.hpp"
   31 #include "services/memoryUsage.hpp"
   32 #endif // INCLUDE_EPSILONGC

The #if INCLUDE_EPSILONGC shouldn't be needed here.


src/hotspot/share/gc/shared/vmStructs_gc.hpp
src/hotspot/share/gc/shared/barrierSetConfig.hpp
src/hotspot/share/gc/shared/barrierSetConfig.inline.hpp
src/hotspot/share/gc/shared/gcConfig.cpp
src/hotspot/share/gc/shared/gc_globals.hpp
-------------------------------------------------------

Please keep the #if INCLUDE_XXX, XXX_ONLY and NOT_XXX sections sorted 
alphabetically. I.e. CMS - Epsilon - G1 ...


cheers,
/Per

> 
> Notes:
>    *) C2 barrier modularization had landed, and now Epsilon has no platform-specific impact (this
> alone makes me impressed and happy);
>    *) Elastic TLAB machinery is now able to decay TLAB sizes as well, cutting down memory footprint
> on bursty allocations, more tests for it added, gating by VM options implemented;
>    *) Serviceability support implemented, verified with ad-hoc hsdb session ("universe" and
> "scanoops" work as expected), and serviceability/sa tests;
>    *) Tests are properly keyed with vm.gc.Epsilon, so they are ignored if Epsilon is not built
> 
> Builds:
>      server X {x86_64, x86_32, aarch64, arm32, ppc64le, s390x}
>        zero X {x86_64}
>     minimal X {x86}
> 
> Testing: gc/epsilon on x86_64
> 
> Thanks,
> -Aleksey
>