8266217: ZGC: Improve the -Xlog:gc+init output for NUMA #3778

DamonFool · 2021-04-28T23:02:46Z

Hi all,

This patch improves the -Xlog:gc+init output for NUMA, which is suggested by StefanK [1].
The implementation just follows how UseLargePages are setup and printed.

Before (in docker, not support get_mempolicy)

[0.007s][info][gc,init] Initializing The Z Garbage Collector
[0.007s][info][gc,init] Version: 17-internal+0-adhoc..jdk (fastdebug)
[0.007s][info][gc,init] NUMA Support: Disabled
...

After (in docker, not support get_mempolicy)

[0.007s][info][gc,init] Initializing The Z Garbage Collector
[0.007s][info][gc,init] Version: 17-internal+0-adhoc..jdk (fastdebug)
[0.007s][info][gc,init] NUMA Support: Unsupported
...

Testing:

tier1~3 on Linux/x64, no regression

Thanks.
Best regards,
Jie

[1] https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-March/028927.html

Progress

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

Issue

JDK-8266217: ZGC: Improve the -Xlog:gc+init output for NUMA

Reviewers

Stefan Karlsson (@stefank - Reviewer)
Thomas Schatzl (@tschatzl - Reviewer)
Per Liden (@pliden - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3778

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

Using diff file

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

bridgekeeper · 2021-04-28T23:02:54Z

👋 Welcome back jiefu! 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.

DamonFool · 2021-04-28T23:03:05Z

/test
/label add hotspot-gc
/cc hotspot-gc

openjdk · 2021-04-28T23:04:13Z

@DamonFool
The hotspot-gc label was successfully added.

openjdk · 2021-04-28T23:05:05Z

@DamonFool The hotspot-gc label was already applied.

mlbridge · 2021-04-29T01:34:10Z

Webrevs

00: Full (9b3528f1)

openjdk · 2021-04-29T05:59:50Z

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

8266217: ZGC: Improve the -Xlog:gc+init output for NUMA

Reviewed-by: stefank, tschatzl, pliden

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

155da25: 8265005: Introduce the new client property for mac: apple.awt.windowTitleVisible
91226fa: 8265940: Enable C2's optimization for Math.pow(x, 0.5) on all platforms
56cde70: 8266265: mark hotspot compiler/vectorization tests which ignore VM flags
4937214: 8266174: -Wmisleading-indentation happens in libmlib_image sources
b305eff: 8266238: mark hotspot compiler/inlining tests which ignore VM flags
df3b2d0: 8266264: mark hotspot compiler/eliminateAutobox tests which ignore VM flags
6b263e6: 8266256: compiler.vectorization.TestBufferVectorization does testing twice
69155f4: 8266231: mark hotspot compiler/c1 tests which ignore VM flags
0e225ef: 8197821: Test java/awt/font/TextLayout/LigatureCaretTest.java fails on Windows
a1c942c: 8266170: -Wnonnull happens in classLoaderData.inline.hpp
... and 3 more: https://git.openjdk.java.net/jdk/compare/794cefe8f92ab0c9d5c7c5265232518e8ffb166a...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.

DamonFool · 2021-04-29T06:04:44Z

Thanks @stefank for your review and help.

tschatzl · 2021-04-29T07:14:35Z

src/hotspot/share/gc/z/zNUMA.cpp


 const char* ZNUMA::to_string() {
-  return _enabled ? "Enabled" : "Disabled";
+  switch (_state) {


Maybe consider to have a static const char* strings[] = {"Enabled", "Unsupported", "Disabled" }; (in the correct order) constant here and index by the array (and maybe an assert to catch out of bounds access if/when adds an entry and forgets to update this list).
This is typically more readable then a long switch case like this, but idk other people's opinions.

And of course it's not a particular important suggestion, feel free to ignore.

Maybe consider to have a static const char* strings[] = {"Enabled", "Unsupported", "Disabled" }; (in the correct order) constant here and index by the array (and maybe an assert to catch out of bounds access if/when adds an entry and forgets to update this list).
This is typically more readable then a long switch case like this, but idk other people's opinions.

Thanks @tschatzl for your review.

I didn't think of this skill before.
Thanks for your teaching.

Let's just keep it this time since there are only 3 cases for _state.
And I'd like to use your style of code next time.

Thanks.
Best regards,
Jie

I'd like to keep the current proposal, since it's more inline with what we do in other places in ZGC, like ZLargePages::to_string().

pliden

Looks good.

pliden · 2021-04-29T07:45:49Z

src/hotspot/share/gc/z/zNUMA.cpp


 const char* ZNUMA::to_string() {
-  return _enabled ? "Enabled" : "Disabled";
+  switch (_state) {


I'd like to keep the current proposal, since it's more inline with what we do in other places in ZGC, like ZLargePages::to_string().

DamonFool · 2021-04-29T08:12:27Z

Looks good.

Thanks @pliden .

DamonFool · 2021-04-29T23:05:51Z

/integrate

openjdk · 2021-04-29T23:06:56Z

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

5d8c1cc: 8255410: Add ChaCha20 and Poly1305 support to SunPKCS11 provider
46b4a14: 8266315: Problem list failing test java/awt/font/TextLayout/LigatureCaretTest.java
42af7da: 8265933: Move Java monitor related fields from class Thread to JavaThread
1afbab6: 8263998: Remove mentions of mc region in comments
51b2fb5: 8266299: ProblemList runtime/stringtable/StringTableCleaningTest.java on linux-aarch64 with ZGC
49d0458: 8266288: assert root method not found in witnessed_reabstraction_in_supers is too strong
01415f3: 8266250: WebSocketTest and WebSocketProxyTest call assertEquals(List<byte[]>, List<byte[]>)
5f15666: 8266078: Reader.read(CharBuffer) advances Reader position for read-only Charbuffers
2a03739: 8266014: Regression brought by optimization done with JDK-4926314
6bb71d9: 8264762: ByteBuffer.byteOrder(BIG_ENDIAN).asXBuffer.put(Xarray) and ByteBuffer.byteOrder(nativeOrder()).asXBuffer.put(Xarray) are slow
... and 23 more: https://git.openjdk.java.net/jdk/compare/794cefe8f92ab0c9d5c7c5265232518e8ffb166a...master

Your commit was automatically rebased without conflicts.

Pushed as commit 5ecef01.

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

8266217: ZGC: Improve the -Xlog:gc+init output for NUMA

9b3528f

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Apr 28, 2021

DamonFool marked this pull request as ready for review April 29, 2021 01:29

openjdk bot added the rfr Pull request is ready for review label Apr 29, 2021

stefank approved these changes Apr 29, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 29, 2021

tschatzl reviewed Apr 29, 2021

View reviewed changes

tschatzl approved these changes Apr 29, 2021

View reviewed changes

pliden approved these changes Apr 29, 2021

View reviewed changes

openjdk bot closed this Apr 29, 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 Apr 29, 2021

DamonFool deleted the JDK-8266217 branch April 29, 2021 23:07

8266217: ZGC: Improve the -Xlog:gc+init output for NUMA #3778

8266217: ZGC: Improve the -Xlog:gc+init output for NUMA #3778

Uh oh!

Conversation

DamonFool commented Apr 28, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Apr 28, 2021

Uh oh!

DamonFool commented Apr 28, 2021

Uh oh!

openjdk bot commented Apr 28, 2021

Uh oh!

openjdk bot commented Apr 28, 2021

Uh oh!

mlbridge bot commented Apr 29, 2021

Webrevs

Uh oh!

openjdk bot commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DamonFool commented Apr 29, 2021

Uh oh!

tschatzl Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

tschatzl Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

DamonFool Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

pliden Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

pliden left a comment

Choose a reason for hiding this comment

Uh oh!

pliden Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

DamonFool commented Apr 29, 2021

Uh oh!

DamonFool commented Apr 29, 2021

Uh oh!

openjdk bot commented Apr 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

DamonFool commented Apr 28, 2021 •

edited by openjdk bot

Loading

openjdk bot commented Apr 29, 2021 •

edited

Loading