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

Per Liden per.liden at oracle.com
Fri Nov 30 08:56:16 UTC 2018

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

More information about the build-dev mailing list