JDK-8281460: Let ObjectMonitor have its own NMT category #7378

tstuefe · 2022-02-08T08:10:05Z

It would be useful to distinguish ObjectMonitor allocations in NMT, as a simple way to both to see the needed footprint as well as the number of allocated monitors.

Note, this needs JDK-8281450 as a prerequisite.

Progress

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

Issue

JDK-8281460: Let ObjectMonitor have its own NMT category

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 121811e
Daniel D. Daugherty (@dcubed-ojdk - Reviewer) ⚠️ Review applies to 121811e
Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to 121811e

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7378

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

Using diff file

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

bridgekeeper · 2022-02-08T08:10:12Z

👋 Welcome back stuefe! 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 · 2022-02-08T08:11:29Z

@tstuefe The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-02-08T14:50:24Z

Webrevs

01: Full - Incremental (91a2f54a)
00: Full (121811e9)

dholmes-ora

Hi Thomas,

Looks fine as far as it goes, but leads me to wonder about related data structures e.g. ObjectMonitorsHashtable, which seems to use mtThread. ??

Thanks,
David

tstuefe · 2022-02-09T05:35:28Z

Hi Thomas,

Looks fine as far as it goes, but leads me to wonder about related data structures e.g. ObjectMonitorsHashtable, which seems to use mtThread. ??

Thanks, David

Hi David,

my intent was specifically to get a look at the number of outstanding ObjectMonitor instances. Do you think I am misusing NMT here? If yes, I'm fine with withdrawing the PR.

Cheers, Thomas

dholmes-ora · 2022-02-09T09:07:19Z

Introducing a category just to collect info on a specific kind of allocation does seem somewhat fragile. I can easily imagine someone looking at ObjectMonitorHashtable and thinking it should be tagged too. But in the meantime if this serves a purpose ...

IIRC we have some existing stats we can report about ObjectMonitors if that helps.

Cheers,
David

tstuefe · 2022-02-09T09:44:48Z

Introducing a category just to collect info on a specific kind of allocation does seem somewhat fragile. I can easily imagine someone looking at ObjectMonitorHashtable and thinking it should be tagged too. But in the meantime if this serves a purpose ...

It feels weird now :) but let us see if someone else objects. If yes, I withdraw this change.

shipilev · 2022-02-09T10:03:51Z

It feels weird now :) but let us see if someone else objects. If yes, I withdraw this change.

Yeah, that looks a bit weird, and I have another question: why aren't both ObjectMonitor and ObjectMonitorsHashtable are mtSynchronizer?

tstuefe · 2022-02-09T10:19:47Z

It feels weird now :) but let us see if someone else objects. If yes, I withdraw this change.

Yeah, that looks a bit weird, and I have another question: why aren't both ObjectMonitor and ObjectMonitorsHashtable are mtSynchronizer?

No idea. My guess is that ObjectMonitor is mtInternal because of an oversight in JDK-8253064 and the hashtable is only used as an aid during thread dumps, so mtThread? Would be a question to @dcubed-ojdk.

dholmes-ora · 2022-02-09T12:16:49Z

I think mtSynchronizer was for VM synchronization objects: mutexes, semaphore etc. Not ObjectMonitors. But as always if these conventions are not clearly known then "misuse" can creep in.

dcubed-ojdk · 2022-02-09T17:30:21Z

Actually ObjectMonitor is mtInternal in JDK-8253064 intentionally. Here's the original
code that allocated blocks of ObjectMonitors:

    void* real_malloc_addr = NEW_C_HEAP_ARRAY(char, aligned_size, mtInternal);
    temp = (PaddedObjectMonitor*)align_up(real_malloc_addr, OM_CACHE_LINE_SIZE);
    (void)memset((void *) temp, 0, neededsize);

so the change in allocation implementation in JDK-8253064 is consistent (NMT tag wise)
with what was there before.

That said, I have no complaints about using a specific NMT tag. However, I would
have used "ObjectMonitors" instead of "Monitors" to avoid confusion with VM internal
Monitors. One good reason for this change is that NMT is used for analyzing unexpected
memory usages and since we changed the allocation implementation and some of the
policy in JDK-8253064 it would be good to "highlight" that in NMT as we continue to
shake out what we did in JDK-8253064.

dcubed-ojdk

Thumbs up. I have one minor string rename request that
you can choose to ignore.

dcubed-ojdk · 2022-02-09T17:32:57Z

src/hotspot/share/memory/allocation.hpp

@@ -146,6 +146,7 @@ class AllocatedObj {
  f(mtServiceability, "Serviceability")                                              \
  f(mtMetaspace,      "Metaspace")                                                   \
  f(mtStringDedup,    "String Deduplication")                                        \
+  f(mtObjectMonitor,  "Monitors")                                                    \


I would use "ObjectMonitors" in the string to distinguish from VM internal Monitors.
It also matches the mtObjectMonitor name better.

openjdk · 2022-02-09T17:34:37Z

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

8281460: Let ObjectMonitor have its own NMT category

Reviewed-by: dholmes, dcubed, shade

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 1 new commit pushed to the master branch:

65831eb: 8281318: Improve jfr/event/allocation tests reliability

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 · 2022-02-09T17:38:30Z

Thumbs up. I have one minor string rename request that you can choose to ignore.

Thanks Dan! Your proposal makes sense, I'll change the string before pushing.

tstuefe · 2022-02-10T05:28:38Z

@dholmes-ora @shipilev Dan confirmed my original idea makes sense, are you okay with this change too?

..Thomas

shipilev

All right, but I also keep thinking that NMT becomes a wide-spread diagnostic interface, and at some point in the future we should probably exercise a bit of restraint in introducing overly-fine-grained categories.

dholmes-ora

I'm fine with this as is (modulo Dan's suggested ObjectMonitor change).

I agree with @shipilev that as NMT usage expands we are going to need to more carefully consider how we tag things (and whether we need a way to group tags) so that we can try to address fine- and coarse-grained enquiries.

Cheers,
David

tstuefe · 2022-02-10T07:37:12Z

Alexey:

All right, but I also keep thinking that NMT becomes a wide-spread diagnostic interface, and at some point in the future we should probably exercise a bit of restraint in introducing overly-fine-grained categories.

David:

I'm fine with this as is (modulo Dan's suggested ObjectMonitor change).

I agree with @shipilev that as NMT usage expands we are going to need to more carefully consider how we tag things (and whether we need a way to group tags) so that we can try to address fine- and coarse-grained inquiries.

I agree with both of you. I feel the situation is a bit like with UL tags. We introduced them, but at some point stopped thinking about rules. At least I am not aware of any rules. The result is a bit anarchic. With NMT it is less severe though since with UL, due to the fact how tags are combined when forming log expressions, once they exist you never can change them. NMT categories have fewer backward compatibility headaches.

The obvious exception are people who compare NMT output between JVM releases and get confused about numbers that changed. But that's just education.

NMT group tags were mentioned several times in other PRs, and I think that makes sense and is not that hard to do. Especially when done at a shallow level, just as a display thing (e.g. log with fine granularity categories a,b,c,d, then define "mtInternal" as a group of a,b,c,d, which only has to interest the NMT printing code).

tstuefe · 2022-02-11T07:19:15Z

/integrate

openjdk · 2022-02-11T07:21:11Z

Going to push as commit a037b3c.
Since your change was applied there has been 1 commit pushed to the master branch:

65831eb: 8281318: Improve jfr/event/allocation tests reliability

Your commit was automatically rebased without conflicts.

openjdk · 2022-02-11T07:21:35Z

@tstuefe Pushed as commit a037b3c.

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

mtObjectMonitor

121811e

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Feb 8, 2022

tstuefe changed the title ~~mtObjectMonitor~~ JDK-8281460: Let ObjectMonitor have its own NMT category Feb 8, 2022

tstuefe marked this pull request as ready for review February 8, 2022 14:46

openjdk bot added the rfr Pull request is ready for review label Feb 8, 2022

dholmes-ora reviewed Feb 9, 2022

View reviewed changes

dcubed-ojdk approved these changes Feb 9, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 9, 2022

shipilev approved these changes Feb 10, 2022

View reviewed changes

dholmes-ora approved these changes Feb 10, 2022

View reviewed changes

tstuefe added 2 commits February 11, 2022 07:09

rename category as Dan suggested

feea9a3

Merge

91a2f54

openjdk bot added the integrated Pull request has been integrated label Feb 11, 2022

openjdk bot closed this Feb 11, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 11, 2022

tstuefe deleted the ObjMon-make-its-own-NMT-category branch February 15, 2023 06:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8281460: Let ObjectMonitor have its own NMT category #7378

JDK-8281460: Let ObjectMonitor have its own NMT category #7378

tstuefe commented Feb 8, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 8, 2022

openjdk bot commented Feb 8, 2022

mlbridge bot commented Feb 8, 2022 •

edited

Loading

dholmes-ora left a comment

tstuefe commented Feb 9, 2022

dholmes-ora commented Feb 9, 2022

tstuefe commented Feb 9, 2022

shipilev commented Feb 9, 2022

tstuefe commented Feb 9, 2022

dholmes-ora commented Feb 9, 2022

dcubed-ojdk commented Feb 9, 2022

dcubed-ojdk left a comment

dcubed-ojdk Feb 9, 2022

openjdk bot commented Feb 9, 2022 •

edited

Loading

tstuefe commented Feb 9, 2022

tstuefe commented Feb 10, 2022

shipilev left a comment

dholmes-ora left a comment

tstuefe commented Feb 10, 2022

tstuefe commented Feb 11, 2022

openjdk bot commented Feb 11, 2022

openjdk bot commented Feb 11, 2022

JDK-8281460: Let ObjectMonitor have its own NMT category #7378

JDK-8281460: Let ObjectMonitor have its own NMT category #7378

Conversation

tstuefe commented Feb 8, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 8, 2022

openjdk bot commented Feb 8, 2022

mlbridge bot commented Feb 8, 2022 • edited Loading

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

tstuefe commented Feb 9, 2022

dholmes-ora commented Feb 9, 2022

tstuefe commented Feb 9, 2022

shipilev commented Feb 9, 2022

tstuefe commented Feb 9, 2022

dholmes-ora commented Feb 9, 2022

dcubed-ojdk commented Feb 9, 2022

dcubed-ojdk left a comment

Choose a reason for hiding this comment

dcubed-ojdk Feb 9, 2022

Choose a reason for hiding this comment

openjdk bot commented Feb 9, 2022 • edited Loading

tstuefe commented Feb 9, 2022

tstuefe commented Feb 10, 2022

shipilev left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

tstuefe commented Feb 10, 2022

tstuefe commented Feb 11, 2022

openjdk bot commented Feb 11, 2022

openjdk bot commented Feb 11, 2022

tstuefe commented Feb 8, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 8, 2022 •

edited

Loading

openjdk bot commented Feb 9, 2022 •

edited

Loading