RFR: 8369013: Shenandoah: passive mode should support enabling ShenandoahCardBarrier
William Kemper
wkemper at openjdk.org
Tue Oct 28 17:33:42 UTC 2025
On Fri, 24 Oct 2025 00:18:03 GMT, Rui Li <duke at openjdk.org> wrote:
> Add card barriers to passive mode to test out the price of card barriers.
>
> How this change is implemented is to instantiate the old region in passive mode - old region owns the card table so this would minimize the code change with a bit price of native memory. It does sound weird to have old gen in passive mode, but since passive mode is a just diagnostic mode, we'll go with it for the cleanliness of the change.
This turned out to be simpler than expected. Nice work. I suggest some small changes.
src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.hpp line 47:
> 45:
> 46: static ShenandoahGenerationalHeap* heap() {
> 47: shenandoah_assert_generational();
Can we put these assertions back? I don't see any code in the PR that would invalidate these assertions. I also don't see any changes that would instantiate the generational heap for non generational mode, so if there _is_ code trying to use the generational heap, its behavior will be undefined.
src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 255:
> 253: // for the purpose of having card table.
> 254: if (ShenandoahCardBarrier && !(mode()->is_generational())) {
> 255: _generation_sizer.heap_size_changed(max_capacity());
I think we could simplify here by not using the `_generation_sizer`. It should be fine to pass `max_capacity()` instead of `max_capacity_old` here.
src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 533:
> 531:
> 532: ShenandoahOldGeneration* old_generation() const {
> 533: assert(mode()->is_generational(), "Old generation requires generational mode");
Can we assert that `ShenandoahCardBarrier` is on instead of removing this assertion?
src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 566:
> 564: // For exporting to SA
> 565: int _log_min_obj_alignment_in_bytes;
> 566: ShenandoahGenerationSizer _generation_sizer;
I don't think we need this here.
-------------
Changes requested by wkemper (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/27966#pullrequestreview-3378765228
PR Review Comment: https://git.openjdk.org/jdk/pull/27966#discussion_r2461936571
PR Review Comment: https://git.openjdk.org/jdk/pull/27966#discussion_r2461923434
PR Review Comment: https://git.openjdk.org/jdk/pull/27966#discussion_r2461925051
PR Review Comment: https://git.openjdk.org/jdk/pull/27966#discussion_r2461928806
More information about the hotspot-gc-dev
mailing list