8343957: Rename ObjectMonitor::owner_from() and JavaThread::_lock_id #22524

pchilano · 2024-12-03T19:10:55Z

Please review this small renaming patch. During the review of JDK-8338383 there were some comments about improving the naming for ObjectMonitor::owner_from() and JavaThread::_lock_id. These originate from the changes introduced to inflated monitors, where we now record the java.lang.Thread.tid of the owner in the ObjectMonitor's _owner field instead of a JavaThread*. I renamed _lock_id as _monitor_owner_id and owner_from() as owner_id_from().

Thanks,
Patricio

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-8343957: Rename ObjectMonitor::owner_from() and JavaThread::_lock_id (Enhancement - P4)

Reviewers

Coleen Phillimore (@coleenp - Reviewer)
David Holmes (@dholmes-ora - Reviewer) Review applies to 108abc2b

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22524

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-12-03T19:12:00Z

👋 Welcome back pchilanomate! 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 · 2024-12-03T19:57:19Z

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

8343957: Rename ObjectMonitor::owner_from() and JavaThread::_lock_id

Reviewed-by: coleenp, dholmes

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

faa109e: 8340580: Characters in Document diagrams not in the middle
6aa7667: 8339535: JVM crashes with -Xshare:dump -XX:+SegmentedCodeCache
e1695f6: 8345472: Fix incorrect format instruction for floating point max/min patterns
baa6957: 8345507: Fix build of static launcher
79eb77b: 8345486: Reevaluate the classes in java.lang.classfile.components package
f3b4350: 8345219: C2: x86_64 should not go to interpreter stubs for NaNs handling
16ef6e2: 8344229: Revisit SecurityManager usage in jdk.httpserver after JEP 486 integration
923321c: 8345447: test/jdk/javax/swing/JToolBar/4529206/bug4529206.java fails in ubuntu22.04
6f30762: 8345468: test/jdk/javax/swing/JScrollBar/4865918/bug4865918.java fails in ubuntu22.04
6c1ed0b: 8345474: Translation for instanceof is not triggered when patterns are not used in the compilation unit
... and 64 more: https://git.openjdk.org/jdk/compare/68b1b94d1be686037e2aaef57c0d9adc594fac7a...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.

openjdk · 2024-12-03T19:59:13Z

@pchilano The following labels will be automatically applied to this pull request:

graal
hotspot
serviceability

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 · 2024-12-03T20:11:53Z

Webrevs

01: Full - Incremental (8cfc9a10)
00: Full (108abc2b)

coleenp

Renaming looks good and makes it clearer what the id is.

dholmes-ora

Thanks for making the changes.

One minor nit, but looks good.

dholmes-ora · 2024-12-04T01:36:42Z

src/hotspot/share/runtime/javaThread.hpp

+  void set_monitor_owner_id(int64_t val) {
+    assert(val >= ThreadIdentifier::initial() && val < ThreadIdentifier::current(), "");
+    _monitor_owner_id = val;


Nit: Using id rather than val would be more consistent with other changes (ObjectMonitor::owner_id_from)

dholmes-ora · 2024-12-04T01:42:09Z

src/hotspot/share/runtime/threads.cpp

        if (p->is_vthread_mounted()) {
-          // _lock_id is the thread ID of the mounted virtual thread
-          st->print_cr("   Mounted virtual thread #" INT64_FORMAT, p->lock_id());
+          st->print_cr("   Mounted virtual thread #" INT64_FORMAT, java_lang_Thread::thread_id(p->vthread()));


Was initially unsure why p->lock_id() didn't change to p->monitor_owner_id(), but here you want the thread-id not something that happens to match the thread-id.

coleenp

Update looks good!

pchilano · 2024-12-04T21:24:57Z

Thanks for the reviews Coleen and David!

pchilano · 2024-12-04T21:25:04Z

/integrate

openjdk · 2024-12-04T21:25:39Z

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

ea73e05: 8345181: (ch) Windows asynchronous channels may return temporary direct buffers to the buffer cache twice (win)
ee0f88c: 8341923: java.util.Locale class specification improvements
a72cab8: 8345148: Fix for JDK-8337317 is incomplete
1a73c76: 8345524: CHECK_FOR_FILES should properly handle directories
1d3dcdf: 8345433: (fs) Use stream to load FileTypeDetectors in Files.probeContentType
9740c1d: 8333572: Update jpackage.md with info related to WiX v4 and v5 support
de3a218: 8345502: Remove doIntersectionPrivilege methods
faa109e: 8340580: Characters in Document diagrams not in the middle
6aa7667: 8339535: JVM crashes with -Xshare:dump -XX:+SegmentedCodeCache
e1695f6: 8345472: Fix incorrect format instruction for floating point max/min patterns
... and 71 more: https://git.openjdk.org/jdk/compare/68b1b94d1be686037e2aaef57c0d9adc594fac7a...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-04T21:25:47Z

@pchilano Pushed as commit c113f82.

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

…ds without Pinning Fixes: 78b8015 ("8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning") Fixes: c113f82 ("8343957: Rename ObjectMonitor::owner_from() and JavaThread::_lock_id") Follow-up: 78b8015 ("8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning") Follow-up: c113f82 ("8343957: Rename ObjectMonitor::owner_from() and JavaThread::_lock_id") Link: openjdk#21565 Link: openjdk#22524 Signed-off-by: Bingwu Zhang <[email protected]>

…without Pinning Fixes: 78b8015 ("8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning") Fixes: c113f82 ("8343957: Rename ObjectMonitor::owner_from() and JavaThread::_lock_id") Follow-up: 78b8015 ("8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning") Follow-up: c113f82 ("8343957: Rename ObjectMonitor::owner_from() and JavaThread::_lock_id") Link: openjdk#21565 Link: openjdk#22524 Signed-off-by: Bingwu Zhang <[email protected]>

v1

108abc2

openjdk bot added graal [email protected] serviceability [email protected] hotspot [email protected] labels Dec 3, 2024

pchilano marked this pull request as ready for review December 3, 2024 20:07

openjdk bot added the rfr Pull request is ready for review label Dec 3, 2024

graalvmbot mentioned this pull request Dec 3, 2024

[JDK-8345425] Adapt JDK-8343957: Rename ObjectMonitor::owner_from() and JavaThread::_lock_id oracle/graal#10222

Merged

coleenp approved these changes Dec 3, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 3, 2024

dholmes-ora approved these changes Dec 4, 2024

View reviewed changes

Fix parameter name

8cfc9a1

openjdk bot removed the ready Pull request is ready to be integrated label Dec 4, 2024

coleenp approved these changes Dec 4, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 4, 2024

openjdk bot added the integrated Pull request has been integrated label Dec 4, 2024

openjdk bot closed this Dec 4, 2024

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

graalvmbot mentioned this pull request Dec 9, 2024

[GR-60372] Update labsjdk to 24+27-jvmci-b01 oracle/graal#10269

Merged

8343957: Rename ObjectMonitor::owner_from() and JavaThread::_lock_id #22524

8343957: Rename ObjectMonitor::owner_from() and JavaThread::_lock_id #22524

Uh oh!

Conversation

pchilano commented Dec 3, 2024 • 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 Dec 3, 2024

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Dec 3, 2024

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

pchilano Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

pchilano Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

pchilano commented Dec 4, 2024

Uh oh!

pchilano commented Dec 4, 2024

Uh oh!

openjdk bot commented Dec 4, 2024

Uh oh!

openjdk bot commented Dec 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

pchilano commented Dec 3, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Dec 3, 2024 •

edited

Loading

mlbridge bot commented Dec 3, 2024 •

edited

Loading