8320370: NMT: Change MallocMemorySnapshot to simplify code. #16724

jdksjolen · 2023-11-18T15:48:19Z

Hi,

MallocMemorySnapshot used to be initialized in a quite non-standard way, using global placement new on an array of size_t of the size required to fit MallocMemorySnapshot. This looks like it was intended to circumvent some ResourceObj internals, but I'm unsure of its purpose. This change does what you expect a regular initialization of a global variable to look like.

~~Currently running through tier1.~~ GHA passed and so did tier1 on Oracle CI.

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-8320370: NMT: Change MallocMemorySnapshot to simplify code. (Enhancement - P4)

Reviewers

Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to b0c547b4
Gerard Ziemski (@gerard-ziemski - Committer)
Stefan Karlsson (@stefank - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16724

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-11-18T15:49:36Z

👋 Welcome back jsjolen! 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-11-18T15:50:03Z

@jdksjolen To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels

build
client
compiler
core-libs
graal
hotspot
hotspot-compiler
hotspot-gc
hotspot-jfr
hotspot-runtime
i18n
ide-support
javadoc
jdk
jmx
kulla
net
nio
security
serviceability
shenandoah

jdksjolen · 2023-11-20T09:30:37Z

/label hotspot-runtime

openjdk · 2023-11-20T09:31:29Z

@jdksjolen
The hotspot-runtime label was successfully added.

mlbridge · 2023-11-20T09:34:38Z

Webrevs

tstuefe

Looks good, smaller nits.

tstuefe · 2023-11-20T12:38:32Z

src/hotspot/share/nmt/mallocTracker.cpp

@@ -43,7 +43,7 @@
 #include "utilities/ostream.hpp"
 #include "utilities/vmError.hpp"

-size_t MallocMemorySummary::_snapshot[CALC_OBJ_SIZE_IN_TYPE(MallocMemorySnapshot, size_t)];
+MallocMemorySnapshot MallocMemorySummary::_snapshot{};


Why the curly braces?

They're not necessary, I just have a tendency to explicitly use brace/"uniform" initialization everywhere. I've removed them.

tstuefe · 2023-11-20T12:41:05Z

src/hotspot/share/nmt/virtualMemoryTracker.cpp

@@ -49,9 +49,6 @@ void VirtualMemory::update_peak(size_t size) {
 #endif // ASSERT

 void VirtualMemorySummary::initialize() {
-  assert(sizeof(_snapshot) >= sizeof(VirtualMemorySnapshot), "Sanity Check");
-  // Use placement operator new to initialize static data area.
-  ::new ((void*)_snapshot) VirtualMemorySnapshot();
 }


Can be removed.

Oh, thanks! Should've seen that :-).

tstuefe

Good

openjdk · 2023-11-20T13:41:00Z

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

8320370: NMT: Change MallocMemorySnapshot to simplify code.

Reviewed-by: stuefe, gziemski, stefank

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

58530f4: 8321223: Implementation of Scoped Values (Second Preview)
2f9e70e: 8321001: RISC-V: C2 SignumVF
de95259: 8316454: JFR break locale settings
c42535f: 8321470: ThreadLocal.nextHashCode can be static final
42bb852: 8321467: MemorySegment.setString(long, String, Charset) throws IAE(Misaligned access)
c087e91: 8321505: JFR: Update views
ead4fb1: 8321519: Typo in exception message
b5933b1: 8320805: JFR: Create view for deprecated methods
29d7a22: 8321270: Virtual Thread.yield consumes parking permit
0b0fa47: 8319925: CSS.BackgroundImage incorrectly uses double-checked locking
... and 365 more: https://git.openjdk.org/jdk/compare/03db82818b905f21cb5ad1d56a687e238b4a6e33...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.

tstuefe · 2023-11-20T13:41:43Z

I believe the reason for the original shape of this code was to avoid initialization altogether if NMT was disabled. @zhengyu123 ?

gerard-ziemski

Nice clean up!

Just one question: I'm probably missing something here, but did we have to change from ResourceObj to AnyObj for this change to work?

jdksjolen · 2023-11-21T09:34:52Z

Nice clean up!

Just one question: I'm probably missing something here, but did we have to change from ResourceObj to AnyObj for this change to work?

Hi Gerard, no I think it is unnecessary to do this change to make it work (I haven't checked). I do think keeping it as AnyObj is a good idea however, as MallocMemorySnapshot is never resource allocated.

tstuefe · 2023-11-21T10:02:40Z

Nice clean up!
Just one question: I'm probably missing something here, but did we have to change from ResourceObj to AnyObj for this change to work?

Hi Gerard, no I think it is unnecessary to do this change to make it work (I haven't checked). I do think keeping it as AnyObj is a good idea however, as MallocMemorySnapshot is never resource allocated.

Why not make it a simple object then? Why derive from anything at all?

(Update: I am fine with the current state of the patch; up to you)

jdksjolen · 2023-11-21T13:32:35Z

Nice clean up!
Just one question: I'm probably missing something here, but did we have to change from ResourceObj to AnyObj for this change to work?

Hi Gerard, no I think it is unnecessary to do this change to make it work (I haven't checked). I do think keeping it as AnyObj is a good idea however, as MallocMemorySnapshot is never resource allocated.

Why not make it a simple object then? Why derive from anything at all?

(Update: I am fine with the current state of the patch; up to you)

It's not clear to me what the rules are on this matter. To what degree are plain global classes allowed in HotSpot? I thought that they all need to inherit from AnyObj,CHeapObj, etc. Seems like we're more OK with internal (structs and classes within other classes) being "undecorated" with their allocation capabilities.

tstuefe · 2023-11-21T13:50:19Z

Nice clean up!
Just one question: I'm probably missing something here, but did we have to change from ResourceObj to AnyObj for this change to work?

Hi Gerard, no I think it is unnecessary to do this change to make it work (I haven't checked). I do think keeping it as AnyObj is a good idea however, as MallocMemorySnapshot is never resource allocated.

Why not make it a simple object then? Why derive from anything at all?
(Update: I am fine with the current state of the patch; up to you)

It's not clear to me what the rules are on this matter. To what degree are plain global classes allowed in HotSpot? I thought that they all need to inherit from AnyObj,CHeapObj, etc. Seems like we're more OK with internal (structs and classes within other classes) being "undecorated" with their allocation capabilities.

I would hope so. Why increase compiler load and force inclusion of unrelated headers for stuff that is really not necessary.
(as I said, I'm fine with the patch as it is. Feel free to push)

zhengyu123 · 2023-11-21T14:15:14Z

I believe the reason for the original shape of this code was to avoid initialization altogether if NMT was disabled. @zhengyu123 ?

I believe it was coded this way to avoid memory allocation (malloc) during NMT initialization, before tracking infrastructure is ready, e.g. MallocSiteTable is initialized.

IIRC, not using static variable was to workaround the order of static variable initialization by linker, cannot remember the details.

stefank · 2023-11-21T14:22:30Z

Nice clean up!
Just one question: I'm probably missing something here, but did we have to change from ResourceObj to AnyObj for this change to work?

Hi Gerard, no I think it is unnecessary to do this change to make it work (I haven't checked). I do think keeping it as AnyObj is a good idea however, as MallocMemorySnapshot is never resource allocated.

Why not make it a simple object then? Why derive from anything at all?

(Update: I am fine with the current state of the patch; up to you)

+1 Please don't use AnyObj unless you really need multiple allocation strategies.

tstuefe · 2023-11-21T15:03:42Z

I believe the reason for the original shape of this code was to avoid initialization altogether if NMT was disabled. @zhengyu123 ?

I believe it was coded this way to avoid memory allocation (malloc) during NMT initialization, before tracking infrastructure is ready, e.g. MallocSiteTable is initialized.

IIRC, not using static variable was to workaround the order of static variable initialization by linker, cannot remember the details.

That makes a lot of sense. Its a moot point now since https://bugs.openjdk.org/browse/JDK-8256844

tstuefe

Still looks good. Nice simplificaiton.

jdksjolen · 2023-11-23T15:45:02Z

Still looks good. Nice simplificaiton.

Thanks for the re-approval!

Nice clean up!
Just one question: I'm probably missing something here, but did we have to change from ResourceObj to AnyObj for this change to work?

Hi Gerard, no I think it is unnecessary to do this change to make it work (I haven't checked). I do think keeping it as AnyObj is a good idea however, as MallocMemorySnapshot is never resource allocated.

Why not make it a simple object then? Why derive from anything at all?
(Update: I am fine with the current state of the patch; up to you)

+1 Please don't use AnyObj unless you really need multiple allocation strategies.

I'm very happy to hear this :).

@gerard-ziemski, would you mind re-approving also?

gerard-ziemski · 2023-11-30T15:48:43Z

Still looks good. Nice simplificaiton.

Thanks for the re-approval!

Nice clean up!
Just one question: I'm probably missing something here, but did we have to change from ResourceObj to AnyObj for this change to work?

Hi Gerard, no I think it is unnecessary to do this change to make it work (I haven't checked). I do think keeping it as AnyObj is a good idea however, as MallocMemorySnapshot is never resource allocated.

Why not make it a simple object then? Why derive from anything at all?
(Update: I am fine with the current state of the patch; up to you)

+1 Please don't use AnyObj unless you really need multiple allocation strategies.

I'm very happy to hear this :).

@gerard-ziemski, would you mind re-approving also?

We currently have:

class MallocMemorySnapshot : public AnyObj {

which used to be:

class MallocMemorySnapshot : public ResourceObj {

Are we OK with using AnyObj here?

stefank

With this change it now seems unnecessary to keep the as_snapshot() functions. Right?

tstuefe · 2023-12-01T05:10:28Z

I'm fine with it either way, but would prefer the object not to inherit from anything if its only used inline or as stack object. In any case, lets ship this.

jdksjolen · 2023-12-07T11:13:45Z

Hi,

Keeping the as_snapshot but removed the inheritance. Thanks for your patience with this PR.

jdksjolen · 2023-12-07T15:57:46Z

/integrate

openjdk · 2023-12-07T15:58:52Z

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

58530f4: 8321223: Implementation of Scoped Values (Second Preview)
2f9e70e: 8321001: RISC-V: C2 SignumVF
de95259: 8316454: JFR break locale settings
c42535f: 8321470: ThreadLocal.nextHashCode can be static final
42bb852: 8321467: MemorySegment.setString(long, String, Charset) throws IAE(Misaligned access)
c087e91: 8321505: JFR: Update views
ead4fb1: 8321519: Typo in exception message
b5933b1: 8320805: JFR: Create view for deprecated methods
29d7a22: 8321270: Virtual Thread.yield consumes parking permit
0b0fa47: 8319925: CSS.BackgroundImage incorrectly uses double-checked locking
... and 365 more: https://git.openjdk.org/jdk/compare/03db82818b905f21cb5ad1d56a687e238b4a6e33...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-12-07T15:59:00Z

@jdksjolen Pushed as commit a7f6016.

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

Make this AnyObj

319b89e

jdksjolen marked this pull request as ready for review November 19, 2023 12:34

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

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Nov 20, 2023

tstuefe reviewed Nov 20, 2023

View reviewed changes

Thomas comments

3b1d782

tstuefe approved these changes Nov 20, 2023

View reviewed changes

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

gerard-ziemski approved these changes Nov 20, 2023

View reviewed changes

Remove AnyObj inheritance

b0c547b

tstuefe approved these changes Nov 23, 2023

View reviewed changes

stefank reviewed Nov 30, 2023

View reviewed changes

Redo

9cd9038

Didn't remove them

b209e88

stefank approved these changes Dec 7, 2023

View reviewed changes

gerard-ziemski approved these changes Dec 7, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Dec 7, 2023

openjdk bot closed this Dec 7, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 7, 2023

8320370: NMT: Change MallocMemorySnapshot to simplify code. #16724

8320370: NMT: Change MallocMemorySnapshot to simplify code. #16724

Uh oh!

Conversation

jdksjolen commented Nov 18, 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 Nov 18, 2023

Uh oh!

openjdk bot commented Nov 18, 2023

Uh oh!

jdksjolen commented Nov 20, 2023

Uh oh!

openjdk bot commented Nov 20, 2023

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

tstuefe left a comment

Choose a reason for hiding this comment

Uh oh!

tstuefe Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

jdksjolen Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

tstuefe Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

jdksjolen Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

tstuefe left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

tstuefe commented Nov 20, 2023

Uh oh!

gerard-ziemski left a comment

Choose a reason for hiding this comment

Uh oh!

jdksjolen commented Nov 21, 2023

Uh oh!

tstuefe commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdksjolen commented Nov 21, 2023

Uh oh!

tstuefe commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhengyu123 commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stefank commented Nov 21, 2023

Uh oh!

tstuefe commented Nov 21, 2023

Uh oh!

tstuefe left a comment

Choose a reason for hiding this comment

Uh oh!

jdksjolen commented Nov 23, 2023

Uh oh!

gerard-ziemski commented Nov 30, 2023

Uh oh!

stefank left a comment

Choose a reason for hiding this comment

Uh oh!

tstuefe commented Dec 1, 2023

Uh oh!

jdksjolen commented Dec 7, 2023

jdksjolen commented Nov 18, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 20, 2023 •

edited

Loading

openjdk bot commented Nov 20, 2023 •

edited

Loading

tstuefe commented Nov 21, 2023 •

edited

Loading

tstuefe commented Nov 21, 2023 •

edited

Loading

zhengyu123 commented Nov 21, 2023 •

edited

Loading