Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8262910: Cleanup THREAD/TRAPS/naming and typing issues in ObjectMonitor and related code #2802

Closed
wants to merge 10 commits into from

Conversation

dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Mar 3, 2021

ObjectMonitors can only be used by JavaThreads (modulo some interactions with hashcodes and deflation) but we use "Thread*" almost everywhere mainly due to use of TRAPS (and TRAPS will itself use JavaThread once JDK-8252685 is done). Also some uses of TRAPS in the API's are wrong as, for example, monitor entry can never throw an exception.

So this cleanup tackles:

  • remove incorrect use of TRAPS
  • change "Thread*" to "JavaThread*" where applicable
  • don't use THREAD for things not related to exception processing
  • standardise the naming so that we have "JavaThread* current" rather a mix if Self/THREAD/jt etc.
  • remove unnecessary as_Java_thread() conversions
  • other misc cleanup I noticed in some functions

The cleanup is predominantly in objectMonitor.* and synchronizer.* but with a fan out to the users of those APIs. No attempt is made to cleanup the callers beyond ensuring we have a suitable JavaThread reference for the calls.

Thanks,
David


Progress

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

Issue

  • JDK-8262910: Cleanup THREAD/TRAPS/naming and typing issues in ObjectMonitor and related code

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2802/head:pull/2802
$ git checkout pull/2802

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 3, 2021

👋 Welcome back dholmes! 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
Copy link

openjdk bot commented Mar 3, 2021

@dholmes-ora The following labels will be automatically applied to this pull request:

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

@openjdk openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Mar 3, 2021
@dholmes-ora
Copy link
Member Author

dholmes-ora commented Mar 3, 2021

/label remove hotspot
/label add hotspot-runtime
/label add hotspot-compiler

@openjdk openjdk bot removed the hotspot hotspot-dev@openjdk.org label Mar 3, 2021
@openjdk
Copy link

openjdk bot commented Mar 3, 2021

@dholmes-ora
The hotspot label was successfully removed.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Mar 3, 2021
@openjdk
Copy link

openjdk bot commented Mar 3, 2021

@dholmes-ora
The hotspot-runtime label was successfully added.

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 3, 2021
@openjdk
Copy link

openjdk bot commented Mar 3, 2021

@dholmes-ora
The hotspot-compiler label was successfully added.

@dholmes-ora dholmes-ora marked this pull request as ready for review Mar 3, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 3, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 3, 2021

Webrevs

@@ -1717,7 +1673,7 @@ void ObjectMonitor::wait(jlong millis, bool interruptible, TRAPS) {
// then instead of transferring a thread from the WaitSet to the EntryList
// we might just dequeue a thread from the WaitSet and directly unpark() it.

void ObjectMonitor::INotify(Thread * Self) {
void ObjectMonitor::INotify(JavaThread * current) {
Copy link
Contributor

@coleenp coleenp Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: space between JavaThread and star.

coleenp
coleenp approved these changes Mar 3, 2021
Copy link
Contributor

@coleenp coleenp left a comment

I think this change looks good. I made some general comments on specific lines.

static void enter(Handle obj, BasicLock* lock, TRAPS);
static void exit(oop obj, BasicLock* lock, Thread* THREAD);
static void enter(Handle obj, BasicLock* lock, JavaThread* current);
static void exit(oop obj, BasicLock* lock, TRAPS);
Copy link
Contributor

@coleenp coleenp Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So are you suggesting that we should have the convention that if a function takes a Thread/JavaThread argument as the last argument, to use "current" rather than "THREAD", since the latter implies it is supposed to be used to pass an argument for the parameter to TRAPS? I think this is good. It manages some confusion about trailing THREAD arguments in some callers. Specializing further to JavaThread from Thread is also a good change.

Copy link
Member

@iklam iklam Mar 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A suggestion (perhaps as a future RFE), if a function never throws:

void foo(Class* c, Method*m, Thread* current);

maybe we should move the last thread argument to first:

void foo(Thread* current, Class* c, Method*m);

This will make it absolutely sure that foo will never throw an exception -- when you are reading a caller of foo, you don't need to refer to the declaration of foo to know that.

Also, this will be consistent with our usual convention of passing "bigger" arguments first (Process > Thread -> Class -> method -> bci, etc).

Copy link
Contributor

@coleenp coleenp Mar 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but I've been and think we should change these as we modified the code, and not wholesale.

Copy link
Member

@iklam iklam Mar 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then maybe we should do the reordering in this PR, since we are changing the parameters anyway.

@@ -143,10 +143,10 @@ class ObjectSynchronizer : AllStatic {
static size_t deflate_idle_monitors();

// Deflate idle monitors:
static void chk_for_block_req(JavaThread* self, const char* op_name,
static void chk_for_block_req(JavaThread* current, const char* op_name,
Copy link
Contributor

@coleenp coleenp Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like "current" better than "self" as a convention.

Copy link
Member

@dcubed-ojdk dcubed-ojdk Mar 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of all the changes from "self" -> "current"... how did you decide to land
a comment on this one... :-)

@openjdk
Copy link

openjdk bot commented Mar 3, 2021

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

8262910: Cleanup THREAD/TRAPS/naming and typing issues in ObjectMonitor and related code

Reviewed-by: coleenp, pchilanomate, dcubed, cjplummer, sspitsyn

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 openjdk bot added the ready Pull request is ready to be integrated label Mar 3, 2021
Copy link
Contributor

@pchilano pchilano left a comment

Hi David,

Changes look good to me.

Thanks,
Patricio


bool enter(TRAPS);
bool enter(JavaThread* current);
void exit(bool not_suspended, TRAPS);
Copy link
Contributor

@pchilano pchilano Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we also remove use of TRAPS for exit()? In case of unbalanced locking we don't throw IMSE, only just assert.

@mlbridge
Copy link

mlbridge bot commented Mar 3, 2021

Mailing list message from David Holmes on hotspot-compiler-dev:

Hi Coleen,

Thanks for looking at this.

On 4/03/2021 2:22 am, Coleen Phillimore wrote:

src/hotspot/share/runtime/objectMonitor.cpp line 1676:

1674: // we might just dequeue a thread from the WaitSet and directly unpark() it.
1675:
1676: void ObjectMonitor::INotify(JavaThread * current) {

nit: space between JavaThread and star.

Fixed - and a couple of other cases too.

David

@mlbridge
Copy link

mlbridge bot commented Mar 3, 2021

Mailing list message from David Holmes on hotspot-compiler-dev:

On 4/03/2021 2:28 am, Coleen Phillimore wrote:

I think this change looks good. I made some general comments on specific lines.

Thanks.

src/hotspot/share/runtime/synchronizer.hpp line 92:

90: // This is the "slow path" version of monitor enter and exit.
91: static void enter(Handle obj, BasicLock* lock, JavaThread* current);
92: static void exit(oop obj, BasicLock* lock, TRAPS);

So are you suggesting that we should have the convention that if a function takes a Thread/JavaThread argument as the last argument, to use "current" rather than "THREAD", since the latter implies it is supposed to be used to pass an argument for the parameter to TRAPS? I think this is good. It manages some confusion about trailing THREAD arguments in some callers. Specializing further to JavaThread from Thread is also a good change.

Yes. As discussed with Ioi, ideally if you see a call:

foo(..., THREAD);

then you should be able to say "Aha! foo is a TRAPS function that can
propagate an exception, but the calling code needs to do some special
handling so isn't using CHECK or CATCH". We are a long way from that due
to incidental uses of THREAD for convenience in things like ResourceMark
construction within TRAPS functions.

Thanks,
David
-----

@mlbridge
Copy link

mlbridge bot commented Mar 3, 2021

Mailing list message from David Holmes on hotspot-compiler-dev:

Hi Patricio,

Thanks for looking at this.

On 4/03/2021 5:26 am, Patricio Chilano Mateo wrote:

Hi David,

Changes look good to me.

Thanks,
Patricio

src/hotspot/share/runtime/objectMonitor.hpp line 306:

304:
305: bool enter(JavaThread* current);
306: void exit(bool not_suspended, TRAPS);

Shouldn't we also remove use of TRAPS for exit()? In case of unbalanced locking we don't throw IMSE, only just assert.

Good catch! (no pun intended). Yes I overlooked that was don't actually
throw the IMSE in plain exit (in contrast to jni_exit).

Fixed.

Thanks,
David
-----

@mlbridge
Copy link

mlbridge bot commented Mar 3, 2021

Mailing list message from Coleen Phillimore on hotspot-compiler-dev:

On 3/3/21 5:56 PM, David Holmes wrote:

On 4/03/2021 2:28 am, Coleen Phillimore wrote:

I think this change looks good.? I made some general comments on
specific lines.

Thanks.

src/hotspot/share/runtime/synchronizer.hpp line 92:

90:?? // This is the "slow path" version of monitor enter and exit.
91:?? static void enter(Handle obj, BasicLock* lock, JavaThread*
current);
92:?? static void exit(oop obj, BasicLock* lock, TRAPS);

So are you suggesting that we should have the convention that if a
function takes a Thread/JavaThread argument as the last argument, to
use "current" rather than "THREAD", since the latter implies it is
supposed to be used to pass an argument for the parameter to TRAPS??
I think this is good.? It manages some confusion about trailing
THREAD arguments in some callers. Specializing further to JavaThread
from Thread is also a good change.

Yes. As discussed with Ioi, ideally if you see a call:

foo(..., THREAD);

then you should be able to say "Aha! foo is a TRAPS function that can
propagate an exception, but the calling code needs to do some special
handling so isn't using CHECK or CATCH". We are a long way from that
due to incidental uses of THREAD for convenience in things like
ResourceMark construction within TRAPS functions.

Yes, but please don't change ResourceMark rm(THREAD) to something else.?
It's not worth the churn and nobody is confused by it.
Coleen

Thanks,
David
-----

@pchilano
Copy link
Contributor

pchilano commented Mar 4, 2021

Mailing list message from David Holmes on hotspot-compiler-dev:

Hi Patricio,

Thanks for looking at this.

On 4/03/2021 5:26 am, Patricio Chilano Mateo wrote:

Hi David,
Changes look good to me.
Thanks,
Patricio
src/hotspot/share/runtime/objectMonitor.hpp line 306:

304:
305: bool enter(JavaThread* current);
306: void exit(bool not_suspended, TRAPS);

Shouldn't we also remove use of TRAPS for exit()? In case of unbalanced locking we don't throw IMSE, only just assert.

Good catch! (no pun intended). Yes I overlooked that was don't actually
throw the IMSE in plain exit (in contrast to jni_exit).

Fixed.
Looks good!

Thanks,
Patricio

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

I think I only have nits and typos.
Thanks for doing this massive cleanup!

@@ -258,7 +258,7 @@
volatile_nonstatic_field(ObjectMonitor, _recursions, intptr_t) \
volatile_nonstatic_field(ObjectMonitor, _cxq, ObjectWaiter*) \
volatile_nonstatic_field(ObjectMonitor, _EntryList, ObjectWaiter*) \
volatile_nonstatic_field(ObjectMonitor, _succ, Thread*) \
volatile_nonstatic_field(ObjectMonitor, _succ, JavaThread*) \
Copy link
Member

@dcubed-ojdk dcubed-ojdk Mar 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - please fix the indent before the backslash...

@@ -943,7 +943,7 @@ bool InstanceKlass::link_class_impl(TRAPS) {
{
HandleMark hm(THREAD);
Handle h_init_lock(THREAD, init_lock());
ObjectLocker ol(h_init_lock, THREAD);
ObjectLocker ol(h_init_lock, jt);
Copy link
Member

@dcubed-ojdk dcubed-ojdk Mar 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to mumble about adding a new 'jt' here, but this isn't an
ObjectMonitor related file so you probably held off here.

// ExitSuspendEquivalent:
// A faster alternate to handle_special_suspend_equivalent_condition()
//
// handle_special_suspend_equivalent_condition() unconditionally
// acquires the SR_lock. On some platforms uncontended MutexLocker()
// operations have high latency. Note that in ::enter() we call HSSEC
// while holding the monitor, so we effectively lengthen the critical sections.
//
// There are a number of possible solutions:
//
// A. To ameliorate the problem we might also defer state transitions
// to as late as possible -- just prior to parking.
// Given that, we'd call HSSEC after having returned from park(),
// but before attempting to acquire the monitor. This is only a
// partial solution. It avoids calling HSSEC while holding the
// monitor (good), but it still increases successor reacquisition latency --
// the interval between unparking a successor and the time the successor
// resumes and retries the lock. See ReenterI(), which defers state transitions.
// If we use this technique we can also avoid EnterI()-exit() loop
// in ::enter() where we iteratively drop the lock and then attempt
// to reacquire it after suspending.
//
// B. In the future we might fold all the suspend bits into a
// composite per-thread suspend flag and then update it with CAS().
// Alternately, a Dekker-like mechanism with multiple variables
// would suffice:
// ST Self->_suspend_equivalent = false
// MEMBAR
// LD Self_>_suspend_flags

Copy link
Member

@dcubed-ojdk dcubed-ojdk Mar 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for detecting this stale comment and deleting it.
Also thanks for retiring ExitSuspendEquivalent().

src/hotspot/share/runtime/objectMonitor.cpp Show resolved Hide resolved
src/hotspot/share/runtime/objectMonitor.hpp Show resolved Hide resolved
src/hotspot/share/runtime/synchronizer.hpp Show resolved Hide resolved
@@ -143,10 +143,10 @@ class ObjectSynchronizer : AllStatic {
static size_t deflate_idle_monitors();

// Deflate idle monitors:
static void chk_for_block_req(JavaThread* self, const char* op_name,
static void chk_for_block_req(JavaThread* current, const char* op_name,
Copy link
Member

@dcubed-ojdk dcubed-ojdk Mar 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of all the changes from "self" -> "current"... how did you decide to land
a comment on this one... :-)

src/hotspot/share/runtime/synchronizer.hpp Show resolved Hide resolved
src/hotspot/share/runtime/synchronizer.hpp Show resolved Hide resolved
@mlbridge
Copy link

mlbridge bot commented Mar 5, 2021

Mailing list message from David Holmes on serviceability-dev:

On 5/03/2021 8:54 am, Ioi Lam wrote:

On Thu, 4 Mar 2021 22:25:39 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

A suggestion (perhaps as a future RFE), if a function never throws:

void foo(Class* c, Method*m, Thread* current);

maybe we should move the last `thread` argument to first:

void foo(Thread* current, Class* c, Method*m);

This will make it absolutely sure that `foo` will never throw an exception -- when you are reading a caller of `foo`, you don't need to refer to the declaration of `foo` to know that.

Also, this will be consistent with our usual convention of passing "bigger" arguments first (Process > Thread -> Class -> method -> bci, etc).

I agree, but I've been and think we should change these as we modified the code, and not wholesale.

Then maybe we should do the reordering in this PR, since we are changing the parameters anyway.

I don't agree with this general "rule". Most of the time the Thread* is
not truly a part of the API, just "tramp data" that needs to get passed
through so that we minimise the need to manifest Thread::current. So to
me it is appropriate that this goes at the end.

Cheers,
David

@mlbridge
Copy link

mlbridge bot commented Mar 5, 2021

Mailing list message from David Holmes on serviceability-dev:

Hi Dan,

Thanks for taking a look at this.

Trimming ...

On 5/03/2021 9:46 am, Daniel D.Daugherty wrote:

src/hotspot/share/runtime/synchronizer.hpp line 200:

198: BasicLock _lock;
199: public:
200: ObjectLocker(Handle obj, JavaThread* current);

So no more non-JavaThread uses of ObjectLocker?

Are you aware of any? ObjectLocker (and ObjectWaiter) pertain to
ObjectMonitors, and ObjectMonitors can only be acquired by JavaThreads.
Otherwise this whole change needs to be undone. :)

Thanks,
David

@mlbridge
Copy link

mlbridge bot commented Mar 5, 2021

Mailing list message from David Holmes on serviceability-dev:

Hi again Dan,

(I've given up trying to figure out how PR review emails get split up. :) )

Trimming ...

On 5/03/2021 9:46 am, Daniel D.Daugherty wrote:

src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 261:

259: volatile_nonstatic_field(ObjectMonitor, _cxq, ObjectWaiter*) \
260: volatile_nonstatic_field(ObjectMonitor, _EntryList, ObjectWaiter*) \
261: volatile_nonstatic_field(ObjectMonitor, _succ, JavaThread*) \

nit - please fix the indent before the backslash...

Fixed.

src/hotspot/share/oops/instanceKlass.cpp line 946:

944: HandleMark hm(THREAD);
945: Handle h_init_lock(THREAD, init_lock());
946: ObjectLocker ol(h_init_lock, jt);

I was going to mumble about adding a new 'jt' here, but this isn't an
ObjectMonitor related file so you probably held off here.

Yes, and "jt" already exists as the JavaThread manifestation of Thread.

src/hotspot/share/runtime/objectMonitor.hpp line 49:

47: ObjectWaiter* volatile _next;
48: ObjectWaiter* volatile _prev;
49: JavaThread* _thread;

So no more uses of ObjectWaiter by non-JavaThreads?

See other email. AFAIA non-JavaThreads can wait on ObjectMonitors.

src/hotspot/share/runtime/synchronizer.cpp line 437:

435: BiasedLocking::revoke(obj, current);
436: } else {
437: guarantee(false, "VMThread should not be involved with ObjectMonitor");

Interesting change. Seems out of context for this change.
Since you have "guarantee(false," you can use "fatal(" instead...

This is an incomplete change. Earlier I still had TRAPS on enter() even
though it must be a JavaThread so I changed the BL code fragment to the
above. But then when I realized enter() never produces an exception, the
TRAPS became JavaThread* and so it is impossible to be at a safepoint
(or in the VMThread.). So I've reformulated that block. Thanks for noticing!

src/hotspot/share/runtime/synchronizer.cpp line 612:

610: ObjectMonitor* monitor = inflate(current, obj, inflate_cause_jni_exit);
611: // If this thread has locked the object, exit the monitor. We
612: // intentionally do not use CHECK on check_owner because we must exit the

s/CHECK on check_owner/check_owner/

No, the comment is explaining why we do not have this:

bool owned = monitor->check_owner(CHECK);
if (owned) {
monitor->exit(true, current);
}

the exception we are concerned about is not the one that might be posted
by check_owner, but any pre-existing pending exception that might be
present. We must release the monitor regardless.

src/hotspot/share/runtime/synchronizer.cpp line 678:

676: // after ownership is regained.
677: ObjectMonitor* monitor = inflate(current, obj(), inflate_cause_wait);
678: monitor->wait(0 /* wait-forever */, false /* not interruptible */, current);

Didn't expect to see this change from "millis" to "0" either.
Seems out of context for this change.

Update: I see that you deleted the 'millis' param now. I missed that before.

Yes a "target of opportunity" given the other changes to this method.

src/hotspot/share/runtime/synchronizer.cpp line 1071:

1069: // monitors_iterate() is only called at a safepoint or when the
1070: // target thread is suspended or when the target thread is
1071: // operating on itcurrent. The current closures in use today are

typo - s/itcurrent/itself/

Well spotted! (Did that on purpose just to see who was paying attention
- NOT! :) )

src/hotspot/share/runtime/synchronizer.hpp line 206:

204: void wait(TRAPS) { ObjectSynchronizer::wait(_obj, 0, CHECK); } // wait forever
205: void notify_all(TRAPS) { ObjectSynchronizer::notifyall(_obj, CHECK); }
206: void wait_uninterruptibly(JavaThread* current) { ObjectSynchronizer::wait_uninterruptibly(_obj, current); }

Any reason for not use 'current_thread' here?

?? I'm using "current" everywhere.

src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java line 516:

514: public final int objectMonitorCxq = getFieldOffset("ObjectMonitor::_cxq", Integer.class, "ObjectWaiter*", -1, jdk13Backport);
515: public final int objectMonitorEntryList = getFieldOffset("ObjectMonitor::_EntryList", Integer.class, "ObjectWaiter*", -1, jdk13Backport);
516: public final int objectMonitorSucc = getFieldOffset("ObjectMonitor::_succ", Integer.class, JDK < 17 ? "Thread*" : "JavaThread*", -1, jdk13Backport);

That makes my brain hurt...

Yeah it needs to maintain backward compatibility.

Thanks for the review!

David
-----

@mlbridge
Copy link

mlbridge bot commented Mar 5, 2021

Mailing list message from David Holmes on serviceability-dev:

Could I please get a Thumbs up from serviceability on the two JVMTI
changes, and from compiler folk on the JVMCI and Graal change.

Thanks,
David

On 5/03/2021 2:59 pm, David Holmes wrote:

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Thumbs up.

@mlbridge
Copy link

mlbridge bot commented Mar 9, 2021

Mailing list message from David Holmes on hotspot-compiler-dev:

On 9/03/2021 7:58 pm, Doug Simon wrote:

On Thu, 4 Mar 2021 23:37:27 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

David Holmes has updated the pull request incrementally with one additional commit since the last revision:

More pointer declaration style fixups

src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java line 516:

514: public final int objectMonitorCxq = getFieldOffset("ObjectMonitor::_cxq", Integer.class, "ObjectWaiter*", -1, jdk13Backport);
515: public final int objectMonitorEntryList = getFieldOffset("ObjectMonitor::_EntryList", Integer.class, "ObjectWaiter*", -1, jdk13Backport);
516: public final int objectMonitorSucc = getFieldOffset("ObjectMonitor::_succ", Integer.class, JDK < 17 ? "Thread*" : "JavaThread*", -1, jdk13Backport);

That makes my brain hurt...

That's the price of maintaining Graal to work across 10 (and counting...) JDK versions.
This change looks good to me. Thanks for the heads up @dholmes-ora - I'll port this change to Graal upstream.

Many thanks Doug!

David

Copy link
Contributor

@plummercj plummercj left a comment

JVMTI changes look good.

@mlbridge
Copy link

mlbridge bot commented Mar 9, 2021

Mailing list message from David Holmes on serviceability-dev:

On 10/03/2021 4:55 am, Chris Plummer wrote:

On Fri, 5 Mar 2021 04:59:02 GMT, David Holmes <dholmes at openjdk.org> wrote:

ObjectMonitors can only be used by JavaThreads (modulo some interactions with hashcodes and deflation) but we use "Thread*" almost everywhere mainly due to use of TRAPS (and TRAPS will itself use JavaThread once JDK-8252685 is done). Also some uses of TRAPS in the API's are wrong as, for example, monitor entry can never throw an exception.

So this cleanup tackles:
- remove incorrect use of TRAPS
- change "Thread*" to "JavaThread*" where applicable
- don't use THREAD for things not related to exception processing
- standardise the naming so that we have "JavaThread* current" rather a mix if Self/THREAD/jt etc.
- remove unnecessary as_Java_thread() conversions
- other misc cleanup I noticed in some functions

The cleanup is predominantly in objectMonitor.* and synchronizer.* but with a fan out to the users of those APIs. No attempt is made to cleanup the callers beyond ensuring we have a suitable JavaThread reference for the calls.

Thanks,
David

David Holmes has updated the pull request incrementally with three additional commits since the last revision:

- Fix typo
- Fixed up BiasedLocking code in ObjectSynchronizer::enter
- iFixed alignment in macro

JVMTI changes look good.

Thanks Chris!

David

@sspitsyn
Copy link
Contributor

sspitsyn commented Mar 9, 2021

Hi David,

Nice unification, looks great.
A couple of comments.

src/hotspot/share/runtime/synchronizer.cpp

638 int ObjectSynchronizer::wait(Handle obj, jlong millis, TRAPS) {
639 JavaThread* current = THREAD->as_Java_thread();
. . .
652 DTRACE_MONITOR_WAIT_PROBE(monitor, obj(), current, millis);
653 monitor->wait(millis, true, THREAD); // Not CHECK as we need following code

Is it intentional to use THREAD instead of current at line 653?

1141 bool ObjectSynchronizer::request_deflate_idle_monitors() {
1142 Thread* current = Thread::current();
. . .
1158 if (current->is_Java_thread()) {
1159 // JavaThread has to honor the blocking protocol.
1160 ThreadBlockInVM tbivm(current->as_Java_thread());
. . .

Would it better to define current as at the line 639?
There can be similar cases, e.g. the deflate_idle_monitors().

-Serguei

@mlbridge
Copy link

mlbridge bot commented Mar 10, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Serguei,

Thanks for taking a look.

On 10/03/2021 9:53 am, Serguei Spitsyn wrote:

On Tue, 9 Mar 2021 18:52:27 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

David Holmes has updated the pull request incrementally with three additional commits since the last revision:

- Fix typo
- Fixed up BiasedLocking code in ObjectSynchronizer::enter
- iFixed alignment in macro

JVMTI changes look good.

Hi David,

Nice unification, looks great.
A couple of comments.

src/hotspot/share/runtime/synchronizer.cpp

638 int ObjectSynchronizer::wait(Handle obj, jlong millis, TRAPS) {
639 JavaThread* current = THREAD->as_Java_thread();
. . .
652 DTRACE_MONITOR_WAIT_PROBE(monitor, obj(), current, millis);
653 monitor->wait(millis, true, THREAD); // Not CHECK as we need following code

Is it intentional to use `THREAD` instead of `current` at line 653?

Yes, purely as a documentation aid - the "THREAD" usage indicates the
wait() can lead to an exception. (Once TRAPS is defined using JavaThread
the "current" variable won't be needed.)

1141 bool ObjectSynchronizer::request_deflate_idle_monitors() {
1142 Thread* current = Thread::current();
. . .
1158 if (current->is_Java_thread()) {
1159 // JavaThread has to honor the blocking protocol.
1160 ThreadBlockInVM tbivm(current->as_Java_thread());
. . .

Would it better to define `current` as at the line 639?
There can be similar cases, e.g. the `deflate_idle_monitors()`.

I'm not sure what you mean - the current thread in these calls need not
be a JavaThread ... hmmm actually ... since JDK-8254029
request_deflate_idle_monitors is only used by WhiteBox for testing, so
the current thread must be a JavaThread (previously it could be VMThread
during VM shutdown). But not so for deflate_idle_monitors

// This function is called by the MonitorDeflationThread to deflate
// ObjectMonitors. It is also called via do_final_audit_and_print_stats()
// by the VMThread.
size_t ObjectSynchronizer::deflate_idle_monitors() {

I'll make an adjustment to request_deflate and re-test.

Thanks,
David

@mlbridge
Copy link

mlbridge bot commented Mar 10, 2021

Mailing list message from David Holmes on serviceability-dev:

On 10/03/2021 12:19 pm, Daniel D.Daugherty wrote:

On Wed, 10 Mar 2021 00:57:40 GMT, David Holmes <dholmes at openjdk.org> wrote:

ObjectMonitors can only be used by JavaThreads (modulo some interactions with hashcodes and deflation) but we use "Thread*" almost everywhere mainly due to use of TRAPS (and TRAPS will itself use JavaThread once JDK-8252685 is done). Also some uses of TRAPS in the API's are wrong as, for example, monitor entry can never throw an exception.

So this cleanup tackles:
- remove incorrect use of TRAPS
- change "Thread*" to "JavaThread*" where applicable
- don't use THREAD for things not related to exception processing
- standardise the naming so that we have "JavaThread* current" rather a mix if Self/THREAD/jt etc.
- remove unnecessary as_Java_thread() conversions
- other misc cleanup I noticed in some functions

The cleanup is predominantly in objectMonitor.* and synchronizer.* but with a fan out to the users of those APIs. No attempt is made to cleanup the callers beyond ensuring we have a suitable JavaThread reference for the calls.

Thanks,
David

David Holmes has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision:

- Merge branch 'master' into 8262910-sync-traps
- Updated request_deflate_idle_monitors as it can only be called by a JavaThread via WhiteBox API
- Fix typo
- Fixed up BiasedLocking code in ObjectSynchronizer::enter
- iFixed alignment in macro
- More pointer declaration style fixups
- ObjectMonitor::exit can't actually throw IMSE so replaced TRAPS with JavaThread* current
- Style fix: Thread * -> Thread* in type declarations
- Forgot to updated Graal config for JVMCI structs change
- 8262910: Cleanup THREAD/TRAPS/naming and typing issues in ObjectMonitor and related code

Marked as reviewed by dcubed (Reviewer).

Thanks for the final re-review Dan. I'll push once testing is complete.

David

@sspitsyn
Copy link
Contributor

sspitsyn commented Mar 10, 2021

Thank you for explanations, David.
I did not insist the deflate_idle_monitors has to be changed just wanted to double-check if there is any need.

@dholmes-ora
Copy link
Member Author

dholmes-ora commented Mar 10, 2021

/integrate

@openjdk openjdk bot closed this Mar 10, 2021
@openjdk 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 Mar 10, 2021
@openjdk
Copy link

openjdk bot commented Mar 10, 2021

@dholmes-ora Since your change was applied there have been 15 commits pushed to the master branch:

  • 57f16f9: 8262377: Parallel class resolution loses constant pool error
  • b482733: 8259218: (fs) Add links in from overloaded methods in java.nio.file.Files
  • acda812: 8263333: Improve links from core reflection to JLS and JVMS
  • 9399e1b: 8261918: two runtime/cds/appcds/VerifierTest failed with "Unable to use shared archive"
  • 7e52a6e: 8263380: Unintended use of Objects.nonNull in VarHandles
  • 4b5be40: 8238812: assert(false) failed: bad AD file
  • b2a2ddf: 8262438: sun/security/ssl/SSLLogger/LoggingFormatConsistency.java failed with "SocketException: Socket is closed"
  • c8c0234: 8262471: Fix coding style in src/java.base/share/classes/java/lang/CharacterDataPrivateUse.java
  • 4d21a45: 8262913: KlassFactory::create_from_stream should never return NULL
  • fab5676: 8247869: Change NONCOPYABLE to delete the operations
  • ... and 5 more: https://git.openjdk.java.net/jdk/compare/d0c1aec2023cbaba48548cc22094c417c051595f...master

Your commit was automatically rebased without conflicts.

Pushed as commit c6d74bd.

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

@dholmes-ora dholmes-ora deleted the 8262910 branch Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
8 participants