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

[Per Liden]

On 05/30/2018 12:07 PM, Aleksey Shipilev wrote:
> Thanks for reviews! I made the necessary changes in epsilon-gc-branch in JDK Sandbox, will
> regenerate the webrevs after other reviews complete.
> 
> On 05/30/2018 08:43 AM, Per Liden wrote:
>> 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
>>
>> 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.
> 
> Right! The intent was indeed forcing C2 to test that barriers work there. Fixed:
>    http://hg.openjdk.java.net/jdk/sandbox/rev/98acdabed742
> 
> 
>>> 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.
> 
> When you say "emerging convention", do you the example where this convention is used? This seems to
> be something that is discussed internally at Oracle?
> 
> I always thought that upstream OpenJDK should build all features by default, in order to capture the
> full capacities of the platform, and help developers to avoid bitrot in features. Then, downstream
> builders can select what subset of features they are willing to support. Also, the whole point of
> experimental features is that they are available in default binaries for adopters to try out, albeit
> guarded by -XX:+UnlockExperimentalVMOptions.
> 
> So, until that informational JEP is published, discussed, and accepted by OpenJDK community, I left
> it as is.

Sounds fair. Keep it as is and we'll see later what that JEP proposes.

> 
> 
>>> src/hotspot/share/gc/epsilon/epsilonArguments.cpp
>>> -------------------------------------------------
>>> This section shouldn't need #if INCLUDE_EPSILONGC
>>>
>>> src/hotspot/share/gc/epsilon/epsilonMemoryPool.hpp
>>> --------------------------------------------------
>>>
>>> The #if INCLUDE_EPSILONGC shouldn't be needed here.
> 
> Fixed:
>    http://hg.openjdk.java.net/jdk/sandbox/rev/f1d4d55fc08f
> 
>>> 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.
> 
> Right! No sure what I was thinking:
>    http://hg.openjdk.java.net/jdk/sandbox/rev/43bfde126ee6
> 
>>> 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 ...
> 
> Yup:
>    http://hg.openjdk.java.net/jdk/sandbox/rev/547ded4c0de6

Looks good!

Minor nit, the indentation in vmStructs_gc.hpp looks off by 2 a few places:

+  EPSILONGC_ONLY(VM_STRUCTS_EPSILONGC(nonstatic_field, 
                                                              \
+                                    volatile_nonstatic_field, 
                                                              \
+                                    static_field)) 
                                                              \


+  EPSILONGC_ONLY(VM_TYPES_EPSILONGC(declare_type,                         \
+                                  declare_toplevel_type,                  \
+                                  declare_integer_type))                  \


+  EPSILONGC_ONLY(VM_INT_CONSTANTS_EPSILONGC(declare_constant, 
     \
+                                          declare_constant_with_value)) 
     \


cheers,
Per

> 
> -Aleksey
>