8242847: G1 should not clear mark bitmaps with no marks #5213

walulyai · 2021-08-23T07:51:51Z

Hi all,

Please review this change to bound the range of bitmap clearing for each region using the liveness data collected during marking. For the Concurrent Undo Mark cycle, the liveness information (next_top_at_mark_start and live_words) is in sync wrt. the _next_mark_bitmap that needs clearing. Hence, we use these details to clear only bitmaps for regions that were dirtied and need clearing i.e. only clear between [bottom, ntams), and only clear bitmaps for regions that had at least one bit set (i.e. have some live data).

Testing: Tier 1-3.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8242847: G1 should not clear mark bitmaps with no marks

Reviewers

Thomas Schatzl (@tschatzl - Reviewer)
Stefan Johansson (@kstefanj - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5213/head:pull/5213
$ git checkout pull/5213

Update a local copy of the PR:
$ git checkout pull/5213
$ git pull https://git.openjdk.java.net/jdk pull/5213/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5213

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5213.diff

bridgekeeper · 2021-08-23T07:52:14Z

👋 Welcome back iwalulya! 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 · 2021-08-23T07:54:32Z

@walulyai The following label will be automatically applied to this pull request:

hotspot-gc

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

mlbridge · 2021-08-23T07:59:41Z

Webrevs

tschatzl · 2021-08-23T12:36:45Z

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp

@@ -640,20 +670,21 @@ class G1ClearBitMapTask : public AbstractGangTask {
    G1CollectedHeap::heap()->heap_region_par_iterate_from_worker_offset(&_cl, &_hr_claimer, worker_id);
  }

+


Unnecessary newline.

tschatzl · 2021-08-23T12:39:14Z

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp

+    }
+    return r->end();
+  }
+
  public:
    G1ClearBitmapHRClosure(G1CMBitMap* bitmap, G1ConcurrentMark* cm) : HeapRegionClosure(), _bitmap(bitmap), _cm(cm) {


Since this code is so highly specific to clearing _next_bitmap (and depending on current state which TAMS is the correct one for a given region) I would prefer if the code did not have configurable bitmap too.

tschatzl · 2021-08-23T12:41:47Z

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp

+      assert(_bitmap->get_next_marked_addr(r->next_top_at_mark_start(), r->end()) == r->end(), "Should not have marked bits above ntams");
+      return r->next_top_at_mark_start();
+    }
+    return r->end();


If not in the UndoMark operation (and doing the work concurrently), we could use the prev_top_at_mark_start() here, couldn't we?

Out of curiosity, did you measure what gain (in saved bitmap distance) does using the TAMSes give here? Asking because most TAMSes should be either at the end (=almost the same as top) or bottom of the region, only the current old gen allocation region at the time of the concurrent mark start pause may have a different one (I think).

Maybe that additional suggested (by me) optimization is not worth the effort...

You are right, only in a few cases is TAMS below end, and even then, it is way smaller than chunk_size_in_words.

tschatzl · 2021-08-23T12:43:44Z

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp

@@ -586,24 +586,49 @@ class G1ClearBitMapTask : public AbstractGangTask {
  private:
    G1CMBitMap* _bitmap;
    G1ConcurrentMark* _cm;
+
+  bool has_aborted() {
+    if (_cm != NULL) {


Maybe put the predicate _cm != nullptr in an extra method, something like is_clear_concurrent() (or a better name) would improve readability...

tschatzl

Lgtm.

openjdk · 2021-08-25T09:24:20Z

@walulyai 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:

8242847: G1 should not clear mark bitmaps with no marks

Reviewed-by: tschatzl, sjohanss

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 49 new commits pushed to the master branch:

c5a2712: 8272850: Drop zapping values in the Zap* option descriptions
1e3e333: 8272884: Make VoidClosure::do_void pure virtual
0f428ca: 8272570: C2: crash in PhaseCFG::global_code_motion
b17b821: 8272639: jpackaged applications using microphone on mac
0e7288f: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions
6ace805: 8272856: DoubleFlagWithIntegerValue uses G1GC-only flag
c547ead: 8272916: Copyright year was modified unintentionally in jlink.properties and ImagePluginStack.java
aaedac6: 8265261: java/nio/file/Files/InterruptCopy.java fails with java.lang.RuntimeException: Copy was not interrupted
d34f17c: 8270195: Add missing links between methods of JavaFX properties
f608e81: 8264322: Generate CDS archive when creating custom JDK image
... and 39 more: https://git.openjdk.java.net/jdk/compare/03b5e99d998e037f84e9e2395b49321979c0acd8...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.

kstefanj

Lgtm

walulyai · 2021-08-25T14:32:23Z

Thanks for the reviews.

/integrate

openjdk · 2021-08-25T14:33:41Z

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

2ef6871: 8272447: Remove 'native' ranked Mutex
63e062f: 8236176: Parallel GC SplitInfo comment should be updated for shadow regions
c5a2712: 8272850: Drop zapping values in the Zap* option descriptions
1e3e333: 8272884: Make VoidClosure::do_void pure virtual
0f428ca: 8272570: C2: crash in PhaseCFG::global_code_motion
b17b821: 8272639: jpackaged applications using microphone on mac
0e7288f: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions
6ace805: 8272856: DoubleFlagWithIntegerValue uses G1GC-only flag
c547ead: 8272916: Copyright year was modified unintentionally in jlink.properties and ImagePluginStack.java
aaedac6: 8265261: java/nio/file/Files/InterruptCopy.java fails with java.lang.RuntimeException: Copy was not interrupted
... and 41 more: https://git.openjdk.java.net/jdk/compare/03b5e99d998e037f84e9e2395b49321979c0acd8...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-08-25T14:33:57Z

@walulyai Pushed as commit e36cbd8.

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

walulyai added 4 commits August 20, 2021 11:33

8242847: G1 should not clear mark bitmaps with no marks

1ce43dd

space

e7b2f0b

create private functions

fea0fea

add comments and refactor rename

6ab1f44

openjdk bot added the rfr Pull request is ready for review label Aug 23, 2021

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Aug 23, 2021

tschatzl suggested changes Aug 23, 2021

View reviewed changes

thomas review

0ce053a

openjdk bot removed the rfr Pull request is ready for review label Aug 24, 2021

trailing space

3c7d132

openjdk bot added the rfr Pull request is ready for review label Aug 24, 2021

tschatzl approved these changes Aug 25, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 25, 2021

kstefanj approved these changes Aug 25, 2021

View reviewed changes

openjdk bot closed this Aug 25, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8242847: G1 should not clear mark bitmaps with no marks #5213

8242847: G1 should not clear mark bitmaps with no marks #5213

walulyai commented Aug 23, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 23, 2021

openjdk bot commented Aug 23, 2021

mlbridge bot commented Aug 23, 2021 •

edited

Loading

tschatzl Aug 23, 2021

tschatzl Aug 23, 2021

tschatzl Aug 23, 2021

tschatzl Aug 23, 2021

walulyai Aug 24, 2021

tschatzl Aug 23, 2021

tschatzl left a comment

openjdk bot commented Aug 25, 2021 •

edited

Loading

kstefanj left a comment

walulyai commented Aug 25, 2021

openjdk bot commented Aug 25, 2021

openjdk bot commented Aug 25, 2021

		@@ -640,20 +670,21 @@ class G1ClearBitMapTask : public AbstractGangTask {
		G1CollectedHeap::heap()->heap_region_par_iterate_from_worker_offset(&_cl, &_hr_claimer, worker_id);
		}

8242847: G1 should not clear mark bitmaps with no marks #5213

8242847: G1 should not clear mark bitmaps with no marks #5213

Conversation

walulyai commented Aug 23, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 23, 2021

openjdk bot commented Aug 23, 2021

mlbridge bot commented Aug 23, 2021 • edited Loading

Webrevs

tschatzl Aug 23, 2021

Choose a reason for hiding this comment

tschatzl Aug 23, 2021

Choose a reason for hiding this comment

tschatzl Aug 23, 2021

Choose a reason for hiding this comment

tschatzl Aug 23, 2021

Choose a reason for hiding this comment

walulyai Aug 24, 2021

Choose a reason for hiding this comment

tschatzl Aug 23, 2021

Choose a reason for hiding this comment

tschatzl left a comment

Choose a reason for hiding this comment

openjdk bot commented Aug 25, 2021 • edited Loading

kstefanj left a comment

Choose a reason for hiding this comment

walulyai commented Aug 25, 2021

openjdk bot commented Aug 25, 2021

openjdk bot commented Aug 25, 2021

walulyai commented Aug 23, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 23, 2021 •

edited

Loading

openjdk bot commented Aug 25, 2021 •

edited

Loading