8322503: Shenandoah: Clarify gc state usage #17163

earthling-amzn · 2023-12-19T21:41:04Z

This is a follow up to #17112.

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-8322503: Shenandoah: Clarify gc state usage (Task - P4)

Reviewers

Y. Srinivas Ramakrishna (@ysramakrishna - Reviewer) ⚠️ Review applies to 13fe4565
Guoxiong Li (@lgxbslgx - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17163

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

Using diff file

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

Webrev

Link to Webrev Comment

…e thread local copy of gc state

bridgekeeper · 2023-12-19T21:41:52Z

👋 Welcome back wkemper! 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 · 2023-12-19T21:43:37Z

@earthling-amzn 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 · 2023-12-19T21:46:53Z

Webrevs

ysramakrishna · 2023-12-20T22:03:40Z

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

+  void set_gc_state(uint mask, bool value);

 public:
  char gc_state() const;
-  void set_gc_state_all_threads();
-  bool has_gc_state_changed() { return _gc_state_changed; }
+  void propagate_gc_state_to_java_threads();
+  bool has_gc_state_changed() const { return _gc_state_changed; }


The name changes per @shipilev look good. If we have a sentence of documentation in the header file for each of these methods, that makes things even clearer (and is not subject to limitations on the length of the method name :-)

openjdk · 2023-12-20T22:04:49Z

@earthling-amzn 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:

8322503: Shenandoah: Clarify gc state usage

Reviewed-by: ysr, gli

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

2a59243: 8322734: A redundant return in method padWithLen
4fc6b0f: 8068958: Timestamp.from(Instant) should throw when conversion is not possible
28c82bf: 8322661: Build broken due to missing jvmtiExport.hpp after JDK-8320139
7263e25: 8322490: cleanup CastNode construction
f695ca5: 8321151: JDK-8294427 breaks Windows L&F on all older Windows versions
93fedc1: 8321802: (zipfs) Add validation of incorrect LOC signature in ZipFileSystem
1230853: 8322163: runtime/Unsafe/InternalErrorTest.java fails on Alpine after JDK-8320886
dce7a57: 8321683: Tests fail with AssertionError in RangeWithPageSize
c53f845: 8322539: Parallel: Remove duplicated methods in PSAdaptiveSizePolicy
84c2379: 8320139: [JVMCI] VmObjectAlloc is not generated by intrinsics methods which allocate objects
... and 6 more: https://git.openjdk.org/jdk/compare/f6fe39ff1168d27f4d0ea3e4c7f3f17ecae9e1ab...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@ysramakrishna, @lgxbslgx) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

…owup

earthling-amzn · 2023-12-21T00:21:22Z

/integrate

Added comments to new methods and synced up with tip to see if it addresses GHA build failure on macosx.

openjdk · 2023-12-21T00:21:29Z

@earthling-amzn
Your change (at version db146b8) is now ready to be sponsored by a Committer.

lgxbslgx

Looks good.

ysramakrishna · 2024-01-02T17:06:52Z

/sponsor

openjdk · 2024-01-02T17:08:37Z

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

9481d06: 8320343: Generate GIF images for AbstractButton/5049549/bug5049549.java
a5cf421: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown
f9aec02: 8321540: ClassSignature.parseFrom() throws StringIndexOutOfBoundsException for invalid signatures
7455b1b: 8322159: ThisEscapeAnalyzer crashes for erroneous code
d4fb308: 8317846: Typo in API documentation of classes IdentityHashMap
d786c49: 8322751: ZGC: Fix comments about marking roots
5852f3e: 8322027: One XMLStreamException constructor fails to initialize cause
be0e1c7: 8297573: Parallel: Rename do_oop_nv to do_oop_work in subclasses of OopClosure
7c1d481: 8322765: Eliminate -Wparentheses warnings in runtime code
518ec97: 8322747: StringTable should be AllStatic
... and 18 more: https://git.openjdk.org/jdk/compare/f6fe39ff1168d27f4d0ea3e4c7f3f17ecae9e1ab...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-01-02T17:08:44Z

@ysramakrishna @earthling-amzn Pushed as commit 51238c4.

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

Rename gc state methods for clarity, assert that only java threads us…

13fe456

…e thread local copy of gc state

openjdk bot added the rfr Pull request is ready for review label Dec 19, 2023

openjdk bot added hotspot-gc hotspot-gc-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Dec 19, 2023

ysramakrishna approved these changes Dec 20, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 20, 2023

earthling-amzn added 2 commits December 20, 2023 15:49

Add comments to gc state methods

49ad345

Merge remote-tracking branch 'openjdk/master' into sync-gc-state-foll…

db146b8

…owup

openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 21, 2023

lgxbslgx approved these changes Dec 28, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 2, 2024

openjdk bot closed this Jan 2, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jan 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8322503: Shenandoah: Clarify gc state usage #17163

8322503: Shenandoah: Clarify gc state usage #17163

Uh oh!

earthling-amzn commented Dec 19, 2023 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Dec 19, 2023

Uh oh!

openjdk bot commented Dec 19, 2023

Uh oh!

mlbridge bot commented Dec 19, 2023 •

edited

Loading

Uh oh!

ysramakrishna Dec 20, 2023

Uh oh!

openjdk bot commented Dec 20, 2023 •

edited

Loading

Uh oh!

earthling-amzn commented Dec 21, 2023

Uh oh!

openjdk bot commented Dec 21, 2023

Uh oh!

lgxbslgx left a comment

Uh oh!

ysramakrishna commented Jan 2, 2024

Uh oh!

openjdk bot commented Jan 2, 2024

Uh oh!

openjdk bot commented Jan 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

8322503: Shenandoah: Clarify gc state usage #17163

8322503: Shenandoah: Clarify gc state usage #17163

Uh oh!

Conversation

earthling-amzn commented Dec 19, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Dec 19, 2023

Uh oh!

openjdk bot commented Dec 19, 2023

Uh oh!

mlbridge bot commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

ysramakrishna Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Dec 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

earthling-amzn commented Dec 21, 2023

Uh oh!

openjdk bot commented Dec 21, 2023

Uh oh!

lgxbslgx left a comment

Choose a reason for hiding this comment

Uh oh!

ysramakrishna commented Jan 2, 2024

Uh oh!

openjdk bot commented Jan 2, 2024

Uh oh!

openjdk bot commented Jan 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

earthling-amzn commented Dec 19, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 19, 2023 •

edited

Loading

openjdk bot commented Dec 20, 2023 •

edited

Loading