8315875: GenShen: Remove heap mode check from ShenandoahInitLogger #320

earthling-amzn · 2023-09-07T18:39:00Z

There are some additional improvements to the init logger that should be easy to upstream.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

JDK-8315875: GenShen: Remove heap mode check from ShenandoahInitLogger (Task - P4)

Reviewers

Kelvin Nilsen (@kdnilsen - Committer) ⚠️ Review applies to 9ef0a6d6
Aleksey Shipilev (@shipilev - Committer) ⚠️ Review applies to 9960fe7d
Y. Srinivas Ramakrishna (@ysramakrishna - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 320

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/shenandoah/pull/320.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-09-07T18:40:23Z

👋 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.

earthling-amzn · 2023-09-07T18:45:36Z

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

+
+  ShenandoahHeap* heap = ShenandoahHeap::heap();
+  log_info(gc, init)("Mode: %s", heap->mode()->name());
+  log_info(gc, init)("Heuristics: %s", heap->global_generation()->heuristics()->name());


Note - we still reference the global_generation abstraction in this file and we won't have that upstream. I wonder if we should push that abstraction back into ShenandoahHeap so that this code in ShenandoahInitLogger could call ShenandoahHeap::heuristics directly, with an implementation like:

ShenandoahHeuristics* ShenandoahHeap::heuristics() { return global_generation()->heuristics(); }

Yeah. Your proposed change might make the abstraction a bit cleaner.

mlbridge · 2023-09-07T18:46:52Z

Webrevs

kdnilsen · 2023-09-07T21:45:04Z

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

+
+  ShenandoahHeap* heap = ShenandoahHeap::heap();
+  log_info(gc, init)("Mode: %s", heap->mode()->name());
+  log_info(gc, init)("Heuristics: %s", heap->global_generation()->heuristics()->name());


Yeah. Your proposed change might make the abstraction a bit cleaner.

openjdk · 2023-09-07T21:47:14Z

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

8315875: GenShen: Remove heap mode check from ShenandoahInitLogger

Reviewed-by: kdnilsen, shade, ysr

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

496addb: Merge
024133b: 8311964: Some jtreg tests failing on x86 with error 'unrecognized VM options' (C2 flags)
62a953f: 8315689: G1: Remove unused init_hash_seed
cfc1489: 8315579: SPARC64 builds are broken after JDK-8304913
f6c203e: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API
a01b3fb: 8288660: JavaDoc should be more helpful if it doesn't recognize a tag
ba1a463: 8315377: C2: assert(u->find_out_with(Op_AddP) == nullptr) failed: more than 2 chained AddP nodes?
a258fc4: 8315648: Add test for JDK-8309979 changes
5d3fdc1: 8315612: RISC-V: intrinsic for unsignedMultiplyHigh
5cbff24: 8315406: [REDO] serviceability/jdwp/AllModulesCommandTest.java ignores VM flags
... and 78 more: https://git.openjdk.org/shenandoah/compare/33a8085e044b8a2dfdc909211945a83d89c04afd...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.

shipilev

Looks okay.

ysramakrishna

perfect, thank you!

earthling-amzn · 2023-09-08T17:38:43Z

/integrate

openjdk · 2023-09-08T17:40:55Z

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

496addb: Merge
024133b: 8311964: Some jtreg tests failing on x86 with error 'unrecognized VM options' (C2 flags)
62a953f: 8315689: G1: Remove unused init_hash_seed
cfc1489: 8315579: SPARC64 builds are broken after JDK-8304913
f6c203e: 8314949: linux PPC64 Big Endian: Implementation of Foreign Function & Memory API
a01b3fb: 8288660: JavaDoc should be more helpful if it doesn't recognize a tag
ba1a463: 8315377: C2: assert(u->find_out_with(Op_AddP) == nullptr) failed: more than 2 chained AddP nodes?
a258fc4: 8315648: Add test for JDK-8309979 changes
5d3fdc1: 8315612: RISC-V: intrinsic for unsignedMultiplyHigh
5cbff24: 8315406: [REDO] serviceability/jdwp/AllModulesCommandTest.java ignores VM flags
... and 78 more: https://git.openjdk.org/shenandoah/compare/33a8085e044b8a2dfdc909211945a83d89c04afd...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-09-08T17:41:05Z

@earthling-amzn Pushed as commit 69aa971.

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

earthling-amzn added 2 commits September 1, 2023 15:42

Prototype isolation of shenandoah heap with gc init logger

f64993c

Merge branch 'shenandoah-master' into isolate-shenandoah-heap

9ef0a6d

openjdk bot added the rfr label Sep 7, 2023

earthling-amzn commented Sep 7, 2023

View reviewed changes

kdnilsen approved these changes Sep 7, 2023

View reviewed changes

openjdk bot added the ready label Sep 7, 2023

Hide global generation behind heap

9960fe7

shipilev approved these changes Sep 8, 2023

View reviewed changes

Fix zero build

6e1d7bc

ysramakrishna approved these changes Sep 8, 2023

View reviewed changes

openjdk bot added the integrated label Sep 8, 2023

openjdk bot closed this Sep 8, 2023

openjdk bot removed ready rfr labels Sep 8, 2023

earthling-amzn deleted the isolate-shenandoah-heap branch September 22, 2023 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8315875: GenShen: Remove heap mode check from ShenandoahInitLogger #320

8315875: GenShen: Remove heap mode check from ShenandoahInitLogger #320

earthling-amzn commented Sep 7, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 7, 2023

earthling-amzn Sep 7, 2023

kdnilsen Sep 7, 2023

mlbridge bot commented Sep 7, 2023 •

edited

Loading

kdnilsen Sep 7, 2023

openjdk bot commented Sep 7, 2023 •

edited

Loading

shipilev left a comment

ysramakrishna left a comment

earthling-amzn commented Sep 8, 2023

openjdk bot commented Sep 8, 2023

openjdk bot commented Sep 8, 2023

8315875: GenShen: Remove heap mode check from ShenandoahInitLogger #320

8315875: GenShen: Remove heap mode check from ShenandoahInitLogger #320

Conversation

earthling-amzn commented Sep 7, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 7, 2023

earthling-amzn Sep 7, 2023

Choose a reason for hiding this comment

kdnilsen Sep 7, 2023

Choose a reason for hiding this comment

mlbridge bot commented Sep 7, 2023 • edited Loading

Webrevs

kdnilsen Sep 7, 2023

Choose a reason for hiding this comment

openjdk bot commented Sep 7, 2023 • edited Loading

shipilev left a comment

Choose a reason for hiding this comment

ysramakrishna left a comment

Choose a reason for hiding this comment

earthling-amzn commented Sep 8, 2023

openjdk bot commented Sep 8, 2023

openjdk bot commented Sep 8, 2023

earthling-amzn commented Sep 7, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 7, 2023 •

edited

Loading

openjdk bot commented Sep 7, 2023 •

edited

Loading