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

[Per Liden]

Hi 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