8255019: Shenandoah: Split STW and concurrent mark into separate classes #1009

zhengyu123 · 2020-11-02T14:45:03Z

This is the first part of refactoring, that aims to isolate three Shenandoah GC modes (concurrent, degenerated and full gc).

Shenandoah started with two GC modes, concurrent and full gc, with minimal shared code, mainly in mark phase. After introducing degenerated GC, it shared quite large portion of code with concurrent GC, with the concept that degenerated GC can simply pick up remaining work of concurrent GC in STW mode.

It was not a big problem at that time, since concurrent GC also processed roots STW. Since Shenandoah gradually moved root processing into concurrent phase, code started to diverge, that made code hard to reason and maintain.

First step, I would like to split STW and concurrent mark, so that:

Code has to special case for STW and concurrent mark.
STW mark does not need to rendezvous workers between root mark and the rest of mark
STW mark does not need to activate SATB barrier and drain SATB buffers.
STW mark does not need to remark some of roots.

The patch mainly just shuffles code. Creates a base class ShenandoahMark, and moved shared code (from current shenandoahConcurrentMark) into this base class. I did 'git mv shenandoahConcurrentMark.inline.hpp shenandoahMark.inline.hpp, but git does not seem to reflect that.

A few changes:

Moved task queue set from ShenandoahConcurrentMark to ShenandoahHeap. ShenandoahMark and its subclasses are stateless. Instead, mark states are maintained in task queue, mark bitmap and SATB buffers, so that they can be created on demand.
Split ShenandoahConcurrentRootScanner template to ShenandoahConcurrentRootScanner and ShenandoahSTWRootScanner
Split code inside op_final_mark code into finish_mark and prepare_evacuation helper functions.
Made ShenandoahMarkCompact stack allocated (as well as ShenandoahConcurrentGC and ShenandoahDegeneratedGC in upcoming refactoring)

Progress

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

Issue

JDK-8255019: Shenandoah: Split STW and concurrent mark into separate classes

Reviewers

Roman Kennke (@rkennke - Reviewer) ⚠️ Review applies to c9842ef
Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to 4fbbfd2

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1009/head:pull/1009
$ git checkout pull/1009

bridgekeeper · 2020-11-02T14:45:38Z

👋 Welcome back zgu! 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 · 2020-11-02T14:48:38Z

@zhengyu123 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.

mlbridge · 2020-11-02T14:53:13Z

Webrevs

…JDK-8255019-sh-mark

openjdk · 2020-11-04T15:22:56Z

⚠️ @zhengyu123 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).

rkennke

Overall a great refactoring!
I wonder if we should keep marking state in a single class e.g. ShenandoahMarkingState or even just add it to ShenandoahMarkingContext, instead of scattering it over ShenandoahHeap?

rkennke

Good work! I have a few questions...

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp

src/hotspot/share/gc/shenandoah/shenandoahMarkCompact.cpp

openjdk · 2021-01-04T18:18:53Z

@zhengyu123 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 JDK-8255019-sh-mark
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

shipilev

First read review follows.

src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp

src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp

src/hotspot/share/gc/shenandoah/shenandoahMarkCompact.cpp

src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.inline.hpp

src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp

src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

shipilev · 2021-01-11T17:38:41Z

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

+      if (point == _degenerated_mark) {
+        finish_mark();
+      }


So if we don't call finish_mark, do we ever call set_concurrent_mark_in_progress(false); and mark_complete_marking_context();?

This was not answered, and I don't see relevant follow-up changes. Is this a bug?

No, I don't think so.

For fall through case, STWMark sets both flags.

For degen case, if we pass _degnerated_mark point, op_final_mark calls finish_mark to set them.

src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.hpp

zhengyu123 · 2021-01-13T13:06:19Z

@shade, did I address all your concerns? Thanks.

shipilev

All right, fine, let's do it.

zhengyu123 · 2021-01-14T17:42:13Z

/integrate

openjdk · 2021-01-14T17:43:21Z

@zhengyu123 Since your change was applied there have been 7 commits pushed to the master branch:

aba3431: 8258956: Memory Leak in StringCoding on ThreadLocal resultCached StringCoding.Result
8554fe6: 8253866: Security Libs Terminology Refresh
c2a3c7e: 8259727: Remove redundant "target" arguments to methods in Links
1620664: 8259723: Move Table class to formats.html package
38a1201: 8258912: Remove JVM options CountJNICalls and CountJVMCalls
be57cf1: 8226416: MonitorUsedDeflationThreshold can cause repeated async deflation requests
c822eda: 8259699: Reduce char[] copying in URLEncoder.encode(String, Charset)

Your commit was automatically rebased without conflicts.

Pushed as commit da6bcf9.

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

Zhengyu Gu added 3 commits October 30, 2020 15:13

8255019: Shenandoah: Split STW and concurrent mark into separate classes

4798cc2

Split finish mark and pre-evaculation

787e6ca

Make ShenandoahMarkCompact stack allocated

6b75cf0

openjdk bot added the rfr Pull request is ready for review label Nov 2, 2020

openjdk bot added hotspot-gc hotspot-gc-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Nov 2, 2020

Zhengyu Gu added 4 commits November 2, 2020 10:46

Merge branch 'master' into JDK-8255019-sh-mark

e4113c3

Merge branch 'master' into JDK-8255019-sh-mark

a616a46

Merge

824165f

Merge branch 'JDK-8255019-sh-mark' of github.com:zhengyu123/jdk into …

b4efedb

…JDK-8255019-sh-mark

Zhengyu Gu added 10 commits November 4, 2020 11:43

Move weak reference processing out of STWMark and fix its timings

bb72dea

Merge

9c5d0d3

Merge branch 'master' into JDK-8255019-sh-mark

75e6a85

Merge branch 'master' into JDK-8255019-sh-mark

fe93d95

Merge branch 'master' into JDK-8255019-sh-mark

6896e06

Merge branch 'master' into JDK-8255019-sh-mark

71b38b4

Removed obsoleted class

91886ed

Merge branch 'master' into JDK-8255019-sh-mark

4322490

Merge branch 'master' into JDK-8255019-sh-mark

7524feb

Merge branch 'master' into JDK-8255019-sh-mark

701880d

rkennke reviewed Nov 17, 2020

View reviewed changes

Zhengyu Gu added 3 commits November 18, 2020 14:45

Merge

f65e3da

Moved task queues to marking context

403ec58

Merge

8c58f6f

rkennke suggested changes Nov 20, 2020

View reviewed changes

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp Outdated Show resolved Hide resolved

src/hotspot/share/gc/shenandoah/shenandoahMarkCompact.cpp Show resolved Hide resolved

src/hotspot/share/gc/shenandoah/shenandoahMarkCompact.cpp Show resolved Hide resolved

Merge branch 'master' into JDK-8255019-sh-mark

9301167

Zhengyu Gu added 3 commits December 11, 2020 10:31

Merge branch 'master' into JDK-8255019-sh-mark

85a4469

Merge

cde2011

Update copyright years

4b367ed

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Jan 4, 2021

Merge

b7390c0

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Jan 5, 2021

shipilev suggested changes Jan 5, 2021

View reviewed changes

Zhengyu Gu added 3 commits January 5, 2021 09:55

@shade's comments

8cd3f9d

Silent MacOSX build

dd57c07

Merge branch 'master' into JDK-8255019-sh-mark

a6540b9

shipilev suggested changes Jan 11, 2021

View reviewed changes

Zhengyu Gu added 2 commits January 11, 2021 13:54

Fixes based on shade's comments

37cda8b

Merge branch 'master' into JDK-8255019-sh-mark

4fbbfd2

shipilev approved these changes Jan 14, 2021

View reviewed changes

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Jan 14, 2021

Merge

8a6d94b

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Jan 14, 2021

openjdk bot closed this Jan 14, 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 Jan 14, 2021

zhengyu123 deleted the JDK-8255019-sh-mark branch May 2, 2022 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8255019: Shenandoah: Split STW and concurrent mark into separate classes #1009

8255019: Shenandoah: Split STW and concurrent mark into separate classes #1009

zhengyu123 commented Nov 2, 2020 •

edited by openjdk bot

bridgekeeper bot commented Nov 2, 2020

openjdk bot commented Nov 2, 2020

mlbridge bot commented Nov 2, 2020 •

edited

openjdk bot commented Nov 4, 2020

rkennke left a comment

rkennke left a comment

openjdk bot commented Jan 4, 2021

shipilev left a comment

shipilev Jan 11, 2021

shipilev Jan 13, 2021

zhengyu123 Jan 13, 2021

zhengyu123 commented Jan 13, 2021

shipilev left a comment

zhengyu123 commented Jan 14, 2021

openjdk bot commented Jan 14, 2021

8255019: Shenandoah: Split STW and concurrent mark into separate classes #1009

8255019: Shenandoah: Split STW and concurrent mark into separate classes #1009

Conversation

zhengyu123 commented Nov 2, 2020 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Nov 2, 2020

openjdk bot commented Nov 2, 2020

mlbridge bot commented Nov 2, 2020 • edited

Webrevs

openjdk bot commented Nov 4, 2020

rkennke left a comment

Choose a reason for hiding this comment

rkennke left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 4, 2021

shipilev left a comment

Choose a reason for hiding this comment

shipilev Jan 11, 2021

Choose a reason for hiding this comment

shipilev Jan 13, 2021

Choose a reason for hiding this comment

zhengyu123 Jan 13, 2021

Choose a reason for hiding this comment

zhengyu123 commented Jan 13, 2021

shipilev left a comment

Choose a reason for hiding this comment

zhengyu123 commented Jan 14, 2021

openjdk bot commented Jan 14, 2021

zhengyu123 commented Nov 2, 2020 •

edited by openjdk bot

mlbridge bot commented Nov 2, 2020 •

edited