8210708: Use single mark bitmap in G1

[tschatzl]

Hi all,

can I have reviews for this change that removes one of the mark bitmaps in G1 to save memory?

Previously G1 used two mark bitmaps, one that has been in use for current liveness determination ("prev bitmap"), one that is currently being built ("next bitmap"); these two then were swapped during the remark pause.

G1 needed the current liveness information to determine whether an object (and the corresponding klass) below the respective top-at-mark-start is live, i.e. could be read from.
The mechanism is described in detail in the "Garbage-First Garbage Collection Paper" by Detlefs et al (doi).

This mechanism has several drawbacks:

• you need twice the space than actually necessary as we will see; this space is significant as a bitmap adds ~1.5% of Java heap size to the G1 native memory usage
• during scanning the cards you need to rely a lot on walking the bitmaps (below tams) which is extra work, and actually most of the time it is the case that the tams is at the end of the region

The alternative presented here is to reformat the holes between objects with filler objects; this is a traditional sweep over the heap which has originally been considered fairly expensive. However since JDK 11 G1 already does that sweep anyway when rebuilding remembered sets, and during this investigation it has been shown that reformatting the holes is "for free".

That reformatting not only includes actually stuffing filler objects into these holes, but also updating the BOT concurrently.

This concurrency has some drawback: after class unloading (in the remark pause) and before the Cleanup pause (which indicates the end of the concurrent rebuilding of the remsets and scrubbing the regions phase) using the BOT needs some care:

• during GC actually there is no issue: the BOT is at least stable in that it can not change while accessing it; the BOT may still point to dead objects, but these can be found using the bitmap.
• during concurrent refinement we can still use the BOT, because any mix of old BOT and new BOT eventually leads to some object start, i.e. an object before a given address. However that object may have been overwritten on the heap by filler object data, in a way that the heap is not parsable at the moment. So we must completely rely on the bitmap finding a valid object start from that (preliminary) object start. This observation is the difference to the previous PR#8884 and exploited in this change. This removes a few 100 LOC of changes :)

Above I mentioned that there is a distinction between objects that can be parsed directly and objects for which we need to use the bitmap. For this reason the code introduces a per-region new marker called parsable_bottom (typically called pb in the code, similar to tams) that indicates from which address within the region the heap is parsable.

Let me describe the relation of tams and pb during the various phases in gc:

Young-only gcs (and initial state):
pb = tams = tars = bottom; card scanning always uses the BOT/direct scanning of the heap

bottom  top
|       |  
v       v
+---------------------+
|                     |
+---------------------|
^
|
tams, pb, tars

During Concurrent Mark Start until Remark: tams = top, pb = bottom; concurrent marking runs; we can still use BOT/direct scanning of the heap for refinement and gc card scan.
Marking not only marks objects from bottom to tams, but also sets a flag in the back scan skip table on a fairly coarse basis.

bottom    top
|         |  
v         v
+---------------------+
|                     |
+---------------------|
^         ^
|         |
pb        tams

From Remark -> Cleanup: pb = tams; tams = bottom; tars = top; Remark unloaded classes, so we set pb to tams; in the region at addresses < tams we might encounter unparsable objects. This means that during this time, refinement can not use the BOT to find valid objects spanning into the card it wants to scan as described above. Use the bitmap for that.

Rebuild the remembered sets from bottom to tars, and scrub the heap from bottom to pb.

Note that the change sets pb incrementally (and concurrently) back to bottom as a region finishes scrubbing to decrease the size of the area we need to use the bitmap to help as a BOT replacement.
Scrubbing means: put filler objects in holes of dead objects, and update the BOT according to these filler objects.

bottom        top
|             |  
v             v
+---------------------+
|                     |
+---------------------|
^         ^   ^
|         |   |
tams      pb  (tars)

As soon as scrubbing finishes on a per-region basis: Set pb to bottom - the whole region is now parsable again. At Cleanup, all regions are scrubbed.

Evacuation failure handling: evacuation failure handling has been modified to accomodate this:

• evac failure expects that the bitmap is clear for regions to (intermittently) mark live objects there. This still applies - the young collection needs to clear old gen regions, but only after the Cleanup pause (G1 can only have mixed gcs after cleanup) because only at that point marks may still be left from the marking on the bitmap of old regions. The young gc just clears those (and only those) at the beginning of gc. It also clears the bitmaps of old gen regions in the remove self-forwards phase.
• evac failure of young regions is handled as before: after evacuation failure the region is left as old gen region with marks below tams in the concurrent start pause, otherwise the bitmap is cleared.

Thanks go to @kstefanj for the initial prototype, @walulyai for taking over for a bit. I'll add both as contributors/authors.

Testing: tier1-5 (multiple times), tier 6-8 (running, but the previous version in PR#8884 completed this one fine)

Thanks,
Thomas

---

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-8210708: Use single mark bitmap in G1

Reviewers

• Ivan Walulya (@walulyai - Reviewer)
• Albert Mingkun Yang (@albertnetymk - Reviewer)

Contributors

• Stefan Johansson <sjohanss@openjdk.org>
• Ivan Walulya <iwalulya@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8957

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

Using diff file

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

[tschatzl]

/contributor add @kstefanj
/contributor add iwalulya
/label add hotspot-gc

[bridgekeeper]

👋 Welcome back tschatzl! 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]

@tschatzl
Contributor Stefan Johansson <sjohanss@openjdk.org> successfully added.

[openjdk]

@tschatzl
Contributor Ivan Walulya <iwalulya@openjdk.org> successfully added.

[openjdk]

@tschatzl
The hotspot-gc label was successfully added.

[mlbridge]

Webrevs

• 14: Full (a299ce91)
• 13: Full - Incremental (5f721dc1)
• 12: Full - Incremental (212f8712)
• 11: Full - Incremental (164a52e9)
• 10: Full - Incremental (04a243ee)
• 09: Full (f29c0f1c)
• 08: Full - Incremental (b33a015a)
• 07: Full (0519982f)
• 06: Full - Incremental (afba8ff8)
• 05: Full - Incremental (5f497f3d)
• 04: Full - Incremental (1acc4fcb)
• 03: Full - Incremental (281633d3)
• 02: Full - Incremental (d3a7a96f)
• 01: Full - Incremental (331fb52c)
• 00: Full (8fec420f)

[tschatzl]

Some comments about the latest commits while searching for a bug that has been found out to actually not be caused by this change (but occurs more often):

997354d:

• moved G1BlockOffsetTablePart::update to HeapRegion::update as suggested by @albertnetymk
• renamed G1CollectedHeap::is_marked_next() to G1CollectedHeap::is_marked(), i.e. removing the obsolete next suffix
• merged HeapRegion::live_bytes and HeapRegion::max_live_bytes() as they were equivalent
• moved collector state change whether we are clearing the bitmap from Remark to Cleanup phase
• added assertion about assumption that in HeapRegion::note_self_forwarding_removal_start, outside of the concurrent start pause, TAMS must be bottom (thanks @albertnetymk for the suggestion)
• renamed HeapRegion::obj_is_scrubbed to obj_is_filler and made it private (again, suggested by @albertnetymk )
• optimized `Heapregion::
• factored out a few byte_size(bottom(), top()) calls which are equivalent to the existing HeapRegion::used() call
• optimized HeapRegion::oops_on_memregion_iterate_in_unparsable
• some documentation update

afba8ff:

• improved verification before full gc (still a bit buggy, but not because of it, but because of an oversight earlier)
• removed debug code
• (introduced debug code)

[openjdk]

@tschatzl 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 submit/8210708-drop-single-bitmap5
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[tschatzl]

The latest patch has been checked against PR#9377 and PR#9376.

[walulyai]

Lgtm!

[openjdk]

@tschatzl This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8210708: Use single mark bitmap in G1

Co-authored-by: Stefan Johansson <sjohanss@openjdk.org>
Co-authored-by: Ivan Walulya <iwalulya@openjdk.org>
Reviewed-by: iwalulya, ayang

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 33 new commits pushed to the master branch:

• 8e7b45b: 8282986: Remove "system" in boot class path names
• 74ca6ca: 8289800: G1: G1CollectionSet::finalize_young_part clears survivor list too early
• 86f63f9: 8289164: Convert ResolutionErrorTable to use ResourceHashtable
• 013a5ee: 8137280: Remove eager reclaim of humongous controls
• 77ad998: 8289778: ZGC: incorrect use of os::free() for mountpoint string handling after JDK-8289633
• 532a6ec: 7124313: [macosx] Swing Popups should overlap taskbar
• e05b2f2: 8289856: [PPC64] SIGSEGV in C2Compiler::init_c2_runtime() after JDK-8289060
• cce77a7: 8289799: Build warning in methodData.cpp memset zero-length parameter
• d1249aa: 8198668: MemoryPoolMBean/isUsageThresholdExceeded/isexceeded001/TestDescription.java still failing
• a79ce4e: 8286941: Add mask IR for partial vector operations for ARM SVE
• ... and 23 more: https://git.openjdk.org/jdk/compare/834189527e16d6fc3aedb97108b0f74c391dbc3b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[tschatzl]

Thanks a lot @albertnetymk @walulyai for your reviews.

/integrate

[openjdk]

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

• 8e7b45b: 8282986: Remove "system" in boot class path names
• 74ca6ca: 8289800: G1: G1CollectionSet::finalize_young_part clears survivor list too early
• 86f63f9: 8289164: Convert ResolutionErrorTable to use ResourceHashtable
• 013a5ee: 8137280: Remove eager reclaim of humongous controls
• 77ad998: 8289778: ZGC: incorrect use of os::free() for mountpoint string handling after JDK-8289633
• 532a6ec: 7124313: [macosx] Swing Popups should overlap taskbar
• e05b2f2: 8289856: [PPC64] SIGSEGV in C2Compiler::init_c2_runtime() after JDK-8289060
• cce77a7: 8289799: Build warning in methodData.cpp memset zero-length parameter
• d1249aa: 8198668: MemoryPoolMBean/isUsageThresholdExceeded/isexceeded001/TestDescription.java still failing
• a79ce4e: 8286941: Add mask IR for partial vector operations for ARM SVE
• ... and 23 more: https://git.openjdk.org/jdk/compare/834189527e16d6fc3aedb97108b0f74c391dbc3b...master

Your commit was automatically rebased without conflicts.

[openjdk]

@tschatzl Pushed as commit 95e3190.

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

[ysramakrishna]

Do you have any before-after performance numbers for the change? (If so, would make sense to add to the JBS ticket.)