Skip to content

Conversation

@kdnilsen
Copy link
Contributor

@kdnilsen kdnilsen commented Aug 20, 2025

This PR eliminates redundant bookkeeping that had been carried out by both ShenandoahGeneration and ShenandoahFreeSet. In the new code, we keep a single tally of relevant information within ShenandoahFreeSet.
Queries serviced by ShenandoahGeneration are now delegated to ShenandoahFreeSet.

This change eliminates rare and troublesome assertion failures that were often raised when the ShenandoahFreeSet tallies did not match the ShenandoahGeneration tallies. These assertion failures resulted because the two sets of books are updated at different times, using different synchronization mechanisms.

The other benefit of this change is that we have less synchronization overhead because we only have to maintain a single set of books.


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-8365880: Shenandoah: Unify memory usage accounting in ShenandoahFreeSet (Task - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26867

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

Using diff file

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

Using Webrev

Link to Webrev Comment

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Oct 18, 2025
@openjdk openjdk bot added the serviceability serviceability-dev@openjdk.org label Oct 21, 2025
@openjdk
Copy link

openjdk bot commented Oct 21, 2025

@kdnilsen serviceability has been added to this pull request based on files touched in new commit(s).

@openjdk openjdk bot removed the rfr Pull request is ready for review label Oct 23, 2025
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 24, 2025
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Oct 24, 2025
@kdnilsen
Copy link
Contributor Author

I'm not convinced the change above to move the constraints check sooner is needed in order to use the value of SoftMaxHeapSize in ShenandoahHeap::initialize().

What's the error you see without this change?

I did some testing after reverting my change to the shared code, and I wasn't able to identify exactly how this change relates to particular regression failures. I've committed new changes to mitigate the issues without changing shared code:

  1. Inside ShenandoahHeap::initialize(), we clamp the assignment to _soft_max_size between min_capacity() and max_capacity()
  2. In ShenandoahHeap::post_initialize(), we check_soft_max_changed(). This will change the value of _soft_max_size if the current value of SoftMaxHeapSize, which has now had its constraints enforced, differs from the current value of _soft_max_size. Constraints are enforced following initialize() but before post_initialize().

int exitValue;
do {
exitValue = run(pb);
} while ((exitValue != 0) && (allowed_retries-- > 0));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nit, but the other variables here use Java's camel case convention, so should probably have allowedRetries, not allowed_retries.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 31, 2025
@kdnilsen
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Oct 31, 2025

Going to push as commit ec059c0.
Since your change was applied there have been 14 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 31, 2025
@openjdk openjdk bot closed this Oct 31, 2025
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 31, 2025
@openjdk
Copy link

openjdk bot commented Oct 31, 2025

@kdnilsen Pushed as commit ec059c0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

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

Labels

hotspot-gc hotspot-gc-dev@openjdk.org integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org

Development

Successfully merging this pull request may close these issues.

3 participants