8353115: GenShen: mixed evacuation candidate regions need accurate live_data

[kdnilsen]

The existing implementation of get_live_data_bytes() and git_live_data_words() does not always behave as might be expected. In particular, the value returned ignores any allocations that occur subsequent to the most recent mark effort that identified live data within the region. This is typically ok for young regions, where the amount of live data determines whether a region should be added to the collection set during the final-mark safepoint.

However, old-gen regions that are placed into the set of candidates for mixed evacuation are more complicated. In particular, by the time the old-gen region is added to a mixed evacuation, its live data may be much larger than at the time concurrent old marking ended.

This PR provides comments to clarify the shortcomings of the existing functions, and adds new functions that provide more accurate accountings of live data for mixed-evacuation candidate regions.

---

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-8353115: GenShen: mixed evacuation candidate regions need accurate live_data (Task - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24319

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[kdnilsen]

I have placed instrumentation into the code to confirm that the live_data reported by ShenandoahHeapRegion is the same before and after this PR for traditional Shenandoah mode. So the regressions are either "signal noise", or perhaps inefficiencies introduced regarding how we compute the live_data.

[openjdk]

@kdnilsen this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout fix-live-data-for-mixed-evac-candidates
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[kdnilsen]

After refactoring the code to perform better in the "tight" rebuild free-set and build-collection set loops, the Shenandoah results show very slight improvement (rather than regression) on specjbb2015. Here is the performance regression that we saw before commit ecdec63

Here are comparisons (in a slightly different environment) after that same commit:

[earthling-amzn]

Why do we want to change this signature? If index is always this->_index why go through the trouble to pass a member field to a member function on the same instance? I have a similar sentiment about passing ShenandoahMarkingContext through the function. Should we have a member ShenandoahMarkingContext* _marking_context? Changing this signature creates a lot of noise on the PR and it's not clear to me why we would do this.

[kdnilsen]

Good call out. Am willing to back this change out. Motivation for this change is that we were seeing some performance regression in this PR (especially noticeable on traditional Shenandoah). At first, I thought this was due to miscomputation of get_live_data_words(), but I confirmed through further testing that the results from get_live_data_words() were the same before and after this PR.

So I concluded that the "explanation" for performance regression is that it now takes longer for us to compute get_live_data_words(). The original implementation was:

  return AtomicAccess::load(&_live_data)

The new implementation added:

  Find the marking context by fetching this from ShenandoahHeap::heap()
  Find tams by consulting the marking context with region, which has to indirect through region to find index

I found that passing this information into the function rather than having the function recompute it brought us back to par with performance of master.

Functions are declared in-line, and it is conceivable that the compiler would figure this crude optimization out for itself, but it didn't.

[ysramakrishna]

What if one used "garbage" as the sorting metric for efficiency (under assumption that I stated earlier of considering only retired, fully allocated regions -- the alternative makes the metric a bit more nuanced), and compute garbage as [regionSize(or used assuming all of region allocated) - markedLive]. This makes the metric invariant after final marking for any region considered in the target evacuation set, and you don't deal with trying to determine the amount allocated above TAMS, keeping the calculations simple and the selection and sorting criteria clean and easy to reason about.

I also noticed that choosing selection set etc. takes the heap lock. Why?

I'll leave more specific comments in the code later today.

[bridgekeeper]

@kdnilsen This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!