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

[Per Liden]

One more thing I noticed:

There are a couple of tests doing things like this:

   38  * @run main/othervm -Xmx1g -Xbatch -Xcomp 
-XX:-UseTLAB -XX:+UnlockExperimentalVMOptions -XX:+UseEpsilonGC 
TestByteArrays
   39  * @run main/othervm -Xmx1g -Xbatch -Xcomp -XX:TieredStopAtLevel=1 
-XX:-UseTLAB -XX:+UnlockExperimentalVMOptions -XX:+UseEpsilonGC 
TestByteArrays
   40  * @run main/othervm -Xmx1g -Xbatch -Xcomp -XX:TieredStopAtLevel=4 
-XX:-UseTLAB -XX:+UnlockExperimentalVMOptions -XX:+UseEpsilonGC 
TestByteArrays

1) Since TieredStopAtLevel=4 is the default, I'm thinking the first and 
last line will test the same thing.

2) I assume "-Xcomp -XX:TieredStopAtLevel=4" is meant to target C2? If 
so, wouldn't "-Xcomp -XX:-TieredCompilation" be a better option? 
Otherwise I suspect most stuff will be compiled with C1 anyway.

cheers,
Per

On 05/30/2018 08:26 AM, Per Liden wrote:
> 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
>>