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

[Per Liden]

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