RFR (round 3), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector

Fri Nov 30 09:34:19 UTC 2018

Hi Roman,

On 11/30/18 10:09 AM, Roman Kennke wrote:
> Hi Per,
> 
> Thanks for taking time to look at this!
> 
> We will take care of all the issues you mentioned.
> 
> Regarding the flags, I think they are useful as they are now. Shenandoah
> can be run in 'passive' mode, which doesn't involve any concurrent GC
> (iow, it runs kindof like Parallel GC). In this mode we can selectively
> turn on/off different kinds of barriers. This is useful:
> - for debugging. if we see a crash and we suspect it's caused by a
> certain type of barrier, we can turn on/off those barriers to narrow down
> - for performance experiments: passive w/o any barriers give a good
> baseline against which we can measure impact of types of barriers.
> 
> Debugging may or may not be useful in develop mode (some bugs only show
> up in product build), performance work quite definitely is not useful in
> develop builds, and therefore I think it makes sense to keep them as
> diagnostic, because that is what they are: diagnostic flags.
> 
> Makes sense?

I can see how these flags can be useful. But I think you might be in 
violating-spec-territory here, if you're allowing users to turn on flags 
that crash the VM. If they are safe to use in 'passive' mode, then I 
think there should be code in shenandoahArguments.cpp that 
rejects/corrects flag combinations that are unstable.

I think the correct way to think about this is: We should pass the TCK, 
regardless of which features are enabled/disabled (unless the VM barfs 
at start-up because the chosen combination of flags are incompatible or 
isn't supported in the current environment, etc).

CC:ing Mikael here, who might be able to shed some light on what's ok 
and not ok here.

cheers,
Per

> 
> Thanks,
> Roman
> 
> 
>> On 11/29/18 9:41 PM, Roman Kennke wrote:
>> [...]
>>> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/03/
>>
>> Some comments (I didn't look at the compiler part):
>>
>>
>> src/hotspot/share/gc/shared/barrierSetConfig.hpp
>> src/hotspot/share/gc/shared/barrierSetConfig.inline.hpp
>> -------------------------------------------------------
>> Please insert the Shenandoah lines so that the lists remain
>> alphabetically sorted on GC names.
>>
>>
>> src/hotspot/share/gc/shared/gcCause.cpp
>> ---------------------------------------
>> Add the Shenandoah stuff after _dcmd_gc_run to match the order in the
>> header file.
>>
>>
>> src/hotspot/share/gc/shared/gcConfig.cpp
>> ----------------------------------------
>> Please insert the Shenandoah lines so that the lists remain
>> alphabetically sorted on GC names. Only the last change is in the right
>> place now.
>>
>>
>> src/hotspot/share/gc/shared/gcTrace.hpp
>> ---------------------------------------
>> Please move this new class to a gc/shenandoah/shanandoahTrace.hpp file
>> (or something similar).
>>
>>
>> src/hotspot/share/gc/shared/gc_globals.hpp
>> src/hotspot/share/gc/shared/vmStructs_gc.hpp
>> ------------------------------------------
>> Please insert the INCLUDE_SHENANDOAHGC/SHENANDOAHGC_ONLY stuff so that
>> the lists remain alphabetically sorted on GC names.
>>
>>
>> src/hotspot/share/utilities/globalDefinitions.hpp
>> -------------------------------------------------
>> I think you want to call the new macro UINT64_FORMAT_X_W, to maintain
>> similarity to it's sibling UINT64_FORMAT_X. Also please adjust the
>> indentation/alignment so the list of macros remains nicely structured.
>>
>>
>> src/hotspot/share/utilities/macros.hpp
>> --------------------------------------
>> Please insert the Shenandoah lines so that the lists remain
>> alphabetically sorted on GC names.
>>
>>
>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/shenandoah/ShenandoahHeap.java
>>
>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/shenandoah/ShenandoahHeapRegion.java
>>
>> -------------------------------------------------------------------------------
>>
>> Since the heap walking is now disabled, it looks like there's quite a
>> bit of code here that can be removed.
>>
>>
>> test/hotspot/jtreg/*
>> --------------------
>> As Aleksey and I discussed in a separate thread. Using
>> 'vm.gc.Shenandoah' does not mean the same thing as 'vm.gc ==
>> "Shenandoah"', and you typically want to use 'vm.gc == "Shenandoah"' in
>> most cases (I did't check all), like in tests that aren't Shenandoah
>> specific.
>>
>>
>> src/hotspot/share/gc/shenandoah/shenandoah_globals.hpp
>> ------------------------------------------------------
>> I don't think you want to expose the following flags. Setting any of
>> them to false will crash the VM, right? If you really want to keep them
>> I'd suggest you make them develop-flags.
>>
>>   346   diagnostic(bool, ShenandoahSATBBarrier, true,          \
>>   347           "Turn on/off SATB barriers in Shenandoah")          \
>>   348          \
>>   349   diagnostic(bool, ShenandoahKeepAliveBarrier, true,          \
>>   350           "Turn on/off keep alive barriers in Shenandoah")          \
>>   351          \
>>   352   diagnostic(bool, ShenandoahWriteBarrier, true,          \
>>   353           "Turn on/off write barriers in Shenandoah")          \
>>   354          \
>>   355   diagnostic(bool, ShenandoahReadBarrier, true,          \
>>   356           "Turn on/off read barriers in Shenandoah")          \
>>   357          \
>>   358   diagnostic(bool, ShenandoahStoreValEnqueueBarrier, false,          \
>>   359           "Turn on/off enqueuing of oops for storeval barriers")
>>           \
>>   360          \
>>   361   diagnostic(bool, ShenandoahStoreValReadBarrier, true,          \
>>   362           "Turn on/off store val read barriers in Shenandoah")
>>           \
>>   363          \
>>   364   diagnostic(bool, ShenandoahCASBarrier, true,          \
>>   365           "Turn on/off CAS barriers in Shenandoah")          \
>>   366          \
>>   367   diagnostic(bool, ShenandoahAcmpBarrier, true,          \
>>   368           "Turn on/off acmp barriers in Shenandoah")          \
>>   369          \
>>   370   diagnostic(bool, ShenandoahCloneBarrier, true,          \
>>   371           "Turn on/off clone barriers in Shenandoah")          \
>>
>>
>> cheers,
>> Per
> 