Skip to content

Conversation

kdnilsen
Copy link
Contributor

@kdnilsen kdnilsen commented Sep 15, 2025

This PR provides several improvements that cumulatively reduce the amount of evacuation performed by GenShen compared to traditional Shenandoah. Specifically:

  1. Do not promote humongous arrays of primitive data. Leaving these regions in the young generation allows them to be reclaimed more quickly when they become garbage. There is no disadvantage to leaving these regions in the young generation as their contents does not need to be scanned by the garbage collector.
  2. Change the default value of ShenandoahOldGarbageThreshold from 15% to 25%. This results in smaller sets of candidate regions for mixed evacuation, less aggressive compaction of old generation. This also increases the likelihood that a region will be promoted in place instead of promoted by evacuation.
  3. Only promote regions in place if the used within a region exceeds ShenandoahGenerationalMinPIPUsage (default 30) percent of the heap region size. This is enforced in combination with the preexisting requirement that the garbage within the region be less than ShenandoahOldGarbageThreshold (new default 25) percent of the heap region size. This causes regions with very low utilization to be promoted by evacuation rather than promoted in place. While the immediate effect of this change is to perform more copying when the region is promoted, the secondary effect is that this region does not need to be evacuated at a later time, after it has accumulated more live data, as part of an effort to defragment old-gen memory.
  4. Only defragment old-gen memory if the command-line has requested non-zero reserves for humongous objects (ShenandoahGenerationalHumongousReserves). Previously, we would invest in defragmenting old-gen memory every time we finished old-generation marking, even if ShenandoahGenerationalHumongousReserves equals zero.

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8367708: GenShen: Reduce total evacuation burden (Task - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27278/head:pull/27278
$ git checkout pull/27278

Update a local copy of the PR:
$ git checkout pull/27278
$ git pull https://git.openjdk.org/jdk.git pull/27278/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27278

View PR using the GUI difftool:
$ git pr show -t 27278

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27278.diff

Using Webrev

Link to Webrev Comment

@kdnilsen kdnilsen marked this pull request as draft September 15, 2025 04:44
@bridgekeeper
Copy link

bridgekeeper bot commented Sep 15, 2025

👋 Welcome back kdnilsen! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Sep 15, 2025

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk
Copy link

openjdk bot commented Sep 15, 2025

⚠️ @kdnilsen This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@openjdk
Copy link

openjdk bot commented Sep 15, 2025

@kdnilsen The following labels will be automatically applied to this pull request:

  • hotspot-gc
  • shenandoah

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@kdnilsen
Copy link
Contributor Author

The impact on overall SpecJBB scores has been minimal.

Compare top charts (proposed PR) with bottom charts (baseline). These charts show the maximum number of bytes evacuated by workers and mutators into the old generation (blue, purple) across all benchmarks (~20 executions of 6 benchmark suites) and platforms (x86, aarch64). Green represents young bytes evacuated by workers; red is young bytes evacuated by mutators. Young evacuations increase slightly with this reduction in old evacuations.

image

@kdnilsen kdnilsen changed the title Combined evac improvements 8367708: GenShen: Reduce total evacuation burden Sep 15, 2025
@kdnilsen kdnilsen marked this pull request as ready for review September 15, 2025 19:49
@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 15, 2025
@mlbridge
Copy link

mlbridge bot commented Sep 15, 2025

Webrevs

Copy link
Contributor

@earthling-amzn earthling-amzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is an accidental change in here that reverts #26256.

assert(start->is_humongous_start(), "reclaim regions starting with the first one");

oop humongous_obj = cast_to_oop(start->bottom());
size_t size = humongous_obj->size();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional? This looks like it would reintroduce #26256 (crash trying to access size of humongous object after its class has been unloaded).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. I'll change how this is implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed this.

@@ -838,7 +838,7 @@ class ShenandoahHeap : public CollectedHeap {
static inline void atomic_clear_oop(narrowOop* addr, oop compare);
static inline void atomic_clear_oop(narrowOop* addr, narrowOop compare);

size_t trash_humongous_region_at(ShenandoahHeapRegion *r) const;
size_t trash_humongous_region_at(ShenandoahHeapRegion *r);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original method was const, removal seems unintentional.

size_t index = region->index();
do {
assert(region->is_humongous(), "Expect correct humongous start or continuation");
// Cannot access humongous_obj->size() in case class has been unloaded
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we want to make two passes over the humongous object? I don't see why the original code needs to change or how this change is related to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a comment in the "new code" that we have to reclaim from the tail. Otherwise assertion fails when printing region to trace log.

I agree it is unclear how this relates to current PR. I'll revert and see if something breaks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. We talked about this comment in the original PR: #26256. It's a stale comment. I ran a pipeline with gc*=trace (and also inspected all usages).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running this change through test pipelines. Perhaps the original problem that I had encountered was fixed in a different way upstream, and my change was no longer necessary or relevant after merging from upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants