8252406: Introduce Thread::as_Java_thread() convenience function

[dholmes-ora]

This is a rather large but generally simple cleanup.

We use a lot of raw C-style casts to "(JavaThread*)" typically after checking "thread->is_Java_thread()". To simplify this pattern, and make the code look somewhat cleaner we introduce a convenience function Thread::as_Java_thread(), which asserts is_Java_thread() and returns "this" cast to "JavaThread*". A const version, as_const_Java_thread(), was also added to allow use in cases where we start with "const Thread *".

Summary of changes:

• changed raw casts to use as_Java_thread()
• removed redundant checks of is_Java_thread() as it is now done in as_Java_thread()
• Removed checks that are checking the type system e.g.

void foo(JavaThread* t) {
  assert(t->is_Java_thread(), "must be")

the only way the assert can fire is if someone deliberately bypasses the type system, and if we are going to worry about that then we would need asserts like this on every method that expects a JavaThread! The right place for the assertion is at the head of any such call chain.

• Removed asserts that are already guaranteed in the caller.
• Use JavaThread::current() where appropriate.
• Use pre-existing "thread" variable where available rather than casting THREAD

Change locations found by grepping for variations of the cast syntax "(JavaThread*)" - it is possible some may have been missed.

Testing: tiers 1-3

Thanks,
David

---

Progress

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

Issue

• JDK-8252406: Introduce Thread::as_Java_thread() convenience function

Reviewers

• Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to 6899517
• Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to 50dd8b3
• Kim Barrett (@kimbarrett - Reviewer)
• Daniel D. Daugherty (@dcubed-ojdk - Reviewer)

Download

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

[bridgekeeper]

👋 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]

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

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 (add|remove) "label" command.

[shipilev]

Drive-by comment: Shenandoah changes look good.

[dholmes-ora]

Thanks @shipilev ! Sending this out for RFR now.

[mlbridge]

Webrevs

• 07: Full - Incremental (f3d2602)
• 06: Full - Incremental (5057562)
• 05: Full - Incremental (9426d53)
• 04: Full - Incremental (50dd8b3)
• 03: Full - Incremental (202af76)
• 02: Full - Incremental (6899517)
• 01: Full - Incremental (da70f80)
• 00: Full (b18faad)

[mlbridge]

Mailing list message from Kim Barrett on shenandoah-dev:

------------------------------------------------------------------------------
src/hotspot/share/runtime/thread.hpp
506 JavaThread* as_Java_thread() {
507 assert(is_Java_thread(), "incorrect cast to JavaThread");
508 return (JavaThread*)this;
509 }

This should be using a static_cast. However, making that change will
uncover a problem.

This definition cannot correctly be located here. It must follow the
definition of the JavaThread class. At this point (in the body of the
Thread class definition), JavaThread is incomplete, so the C-style
cast is a reinterpret_cast. It's only by implementation accident (and
that we're not using multiple or virtual inheritance anywhere in the
vicinity), that a reinterpret_cast happens to work.

(The definition can remain in this file if that's more
convenient #include-wise than putting it in thread.inline.hpp, though
the latter would be the more usual placement.)

(One of the very real dangers with C-style casts is that you might not
be doing the cast you think you are doing.)

------------------------------------------------------------------------------
src/hotspot/share/runtime/thread.hpp
510 JavaThread * as_const_Java_thread() const {
511 assert(is_Java_thread(), "incorrect cast to JavaThread");
512 return (JavaThread*)this;
513 }

This should be

const JavaThread* as_Java_Thread() const {
assert(is_Java_thread(), "incorrect cast to JavaThread");
return static_cast<const JavaThread*>(this);
}

And of course, moved after the definition of JavaThread, per the above
discussion of the non-const function.

------------------------------------------------------------------------------

I reviewed all the (non-Shenandoah) gc changes, and they look good.

[mlbridge]

Mailing list message from David Holmes on shenandoah-dev:

Hi Kim,

Thanks for taking a look.

On 8/09/2020 9:23 am, Kim Barrett wrote:

------------------------------------------------------------------------------
src/hotspot/share/runtime/thread.hpp
506 JavaThread* as_Java_thread() {
507 assert(is_Java_thread(), "incorrect cast to JavaThread");
508 return (JavaThread*)this;
509 }

This should be using a static_cast. However, making that change will
uncover a problem.

This definition cannot correctly be located here. It must follow the
definition of the JavaThread class.

I did try to do the right thing with static_cast and of course
discovered that I couldn't use it at that location. As I was replacing
C-style casts, and as other C-style casts continue to be used in similar
methods, I just kept with the C-styler cast.

At this point (in the body of the
Thread class definition), JavaThread is incomplete, so the C-style
cast is a reinterpret_cast. It's only by implementation accident (and
that we're not using multiple or virtual inheritance anywhere in the
vicinity), that a reinterpret_cast happens to work.

(The definition can remain in this file if that's more
convenient #include-wise than putting it in thread.inline.hpp, though
the latter would be the more usual placement.)

Okay I will look at what is involved in putting it in the .inline.hpp
file; if that causes too many problems then I'll just shift it to an
inline definition later in the hpp file.

(One of the very real dangers with C-style casts is that you might not
be doing the cast you think you are doing.)

------------------------------------------------------------------------------
src/hotspot/share/runtime/thread.hpp
510 JavaThread * as_const_Java_thread() const {
511 assert(is_Java_thread(), "incorrect cast to JavaThread");
512 return (JavaThread*)this;
513 }

This should be

const JavaThread* as_Java_Thread() const {
assert(is_Java_thread(), "incorrect cast to JavaThread");
return static_cast<const JavaThread*>(this);
}

And of course, moved after the definition of JavaThread, per the above
discussion of the non-const function.

Will fix.

------------------------------------------------------------------------------

I reviewed all the (non-Shenandoah) gc changes, and they look good.

Thanks for the review and coding tips!

Cheers,
David

[mlbridge]

Mailing list message from Kim Barrett on shenandoah-dev:

------------------------------------------------------------------------------
src/hotspot/share/runtime/thread.hpp
507 const JavaThread* as_const_Java_thread() const;

This missed part of what I'd suggested. I don't think the "const" part
of the name is needed or helpful. Just overload as_Java_thread on the
const-ness of the thread it's being applied to.

There are some situations where there is such an overload pair and it
is useful to additionally have an explicit const function, but I don't
think this is one of those.

------------------------------------------------------------------------------

The fannout from putting the conversion functions in thread.inline.hpp seems
pretty high in the number of files that get touched. Especially since it
leads to JavaThread::current() and JavaThread::as_CompilerThread() being
moved. I would have been at least fine with, and probably would prefer,
having the definitions of as_JavaThread near the old definitions of those
functions in thread.hpp.

------------------------------------------------------------------------------

[shipilev]

Shenandoah parts still look good. Formally approving.

[openjdk]

@dholmes-ora This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8252406: Introduce Thread::as_Java_thread() convenience function

Reviewed-by: shade, coleenp, kbarrett, dcubed

• If you would like to add a summary, use the /summary command.
• To credit additional contributors, use the /contributor command.
• To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 44 commits pushed to the master branch:

• 4880226: 8171303: sun/java2d/pipe/InterpolationQualityTest.java fails on Windows & Linux
• 8da6c8d: 7183828: Invalid Image Variant when using anything other than BufferedImage
• ff21696: 8252817: Cleanup the classes in the java.awt.color package
• 44a74da: 8252919: JDK built with --enable-cds=no fails with NoClassDefFoundError
• 418e4a2: 8252830: Correct missing javadoc comments in java.rmi module
• 41d29b7: 8252774: remove jdk.test.lib.FileInstaller action from graalunit tests
• 5b30a83: 8252778: remove jdk.test.lib.FileInstaller action from compiler/c2/stemmer test
• 8db3335: 8247928: Refactor G1ConcurrentMarkThread for mark abort
• 7ccf435: 8252846: Fix ppc/s390 after "8231441: AArch64: Initial SVE backend su…
• d236cf4: 8252995: Non-PCH builds broken by JDK-8250961
• ... and 34 more: https://git.openjdk.java.net/jdk/compare/5f76deb2e064ca8a48fb8a638c23aad34bf27f9c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 488022689f690a8b0466b1a10d0a21408f1cbb61.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[mlbridge]

Mailing list message from David Holmes on shenandoah-dev:

On 8/09/2020 4:34 pm, Kim Barrett wrote:

------------------------------------------------------------------------------
src/hotspot/share/runtime/thread.hpp
507 const JavaThread* as_const_Java_thread() const;

This missed part of what I'd suggested. I don't think the "const" part
of the name is needed or helpful. Just overload as_Java_thread on the
const-ness of the thread it's being applied to.

I saw that but thought it was a typo as I don't think about overloads in
terms of const or other modifiers. To me overloads are same named
methods that differ in arguments (and I'm going to guess that 'this' is
considered an argument in this context and hence leads to the overload
based on the const?).

But in addition I wanted to have this say as_const_JavaThread because I
wanted to clearly distringuish it from as_JavaThread because there is
only a couple of special cases in the JFR code where this const version
is needed. If that code were to change and is_const_JavaThread because
unused, it would be easy to see that.

There are some situations where there is such an overload pair and it
is useful to additionally have an explicit const function, but I don't
think this is one of those.

------------------------------------------------------------------------------

The fannout from putting the conversion functions in thread.inline.hpp seems
pretty high in the number of files that get touched. Especially since it
leads to JavaThread::current() and JavaThread::as_CompilerThread() being
moved. I would have been at least fine with, and probably would prefer,
having the definitions of as_JavaThread near the old definitions of those
functions in thread.hpp.

I tried to do the right and obey the "use .inline.hpp for inline
definitions" rule. But I must confess I was reaching a point where I
thought this was going too far but it's a good exercise with using git
and PRs so I don't mind if I back up and take a simpler approach -
especially as I just discovered more build failures due to missing
includes. :(

Thanks,
David

------------------------------------------------------------------------------

[mlbridge]

Mailing list message from Kim Barrett on shenandoah-dev:

On Sep 8, 2020, at 3:00 AM, David Holmes <david.holmes at oracle.com> wrote:

On 8/09/2020 4:34 pm, Kim Barrett wrote:

[?]

src/hotspot/share/runtime/thread.hpp
507 const JavaThread* as_const_Java_thread() const;
This missed part of what I'd suggested. I don't think the "const" part
of the name is needed or helpful. Just overload as_Java_thread on the
const-ness of the thread it's being applied to.

I saw that but thought it was a typo

Intentional, not a typo.

as I don't think about overloads in terms of const or other modifiers. To me overloads are same named methods that differ in arguments (and I'm going to guess that 'this' is considered an argument in this context and hence leads to the overload based on the const?).

That's correct, the function's const qualifier or lack thereof provides
overloading on the corresponding qualification of the 'this' argument.
Such overloading is a "common C++ idiom".

But in addition I wanted to have this say as_const_JavaThread because I wanted to clearly distringuish it from as_JavaThread because there is only a couple of special cases in the JFR code where this const version is needed. If that code were to change and is_const_JavaThread because unused, it would be easy to see that.

I think those cases are just jfr being better than typical HotSpot code
about const qualifications. I wouldn't be surprised if there were other
places that could benefit from having such an overload pair if we tried
harder to be const-correct. So I would prefer overloading rather than the
odd name.

The fannout from putting the conversion functions in thread.inline.hpp seems
pretty high in the number of files that get touched. Especially since it
leads to JavaThread::current() and JavaThread::as_CompilerThread() being
moved. I would have been at least fine with, and probably would prefer,
having the definitions of as_JavaThread near the old definitions of those
functions in thread.hpp.

I tried to do the right and obey the "use .inline.hpp for inline definitions" rule. But I must confess I was reaching a point where I thought this was going too far but it's a good exercise with using git and PRs so I don't mind if I back up and take a simpler approach - especially as I just discovered more build failures due to missing includes. :(

I don't think there is such a rule, but that's a deep discussion that goes
far beyond the scope of this change. I think there are problems with how we
are using "inline" files. Stefan Karlsson and I (and others?) had a long
discussion about this a while back, which I can't find right now (drat!).

[mlbridge]

Mailing list message from David Holmes on shenandoah-dev:

Just to be clear please wait for v5 to appear before reviewing.

Thanks,
David

On 8/09/2020 7:34 pm, David Holmes wrote:

[coleenp]

I'll look at v5 diffs but if I don't, this is still reviewed. Nice cleanup.

[mlbridge]

Mailing list message from David Holmes on shenandoah-dev:

Thanks for the review Coleen!

David

On 9/09/2020 5:10 am, Coleen Phillimore wrote:

[mlbridge]

Mailing list message from Kim Barrett on shenandoah-dev:

On Sep 8, 2020, at 6:06 AM, David Holmes <david.holmes at oracle.com> wrote:

Just to be clear please wait for v5 to appear before reviewing.

I think you meant ?v4?, i.e. the 5th zero-based version?

[mlbridge]

Mailing list message from Kim Barrett on shenandoah-dev:

I reviewed all of v4, not just the gc parts this time.

------------------------------------------------------------------------------
src/hotspot/share/classfile/systemDictionary.cpp

The block from line 1646-1655 seems to be misindented. And was before too.

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/barrierSet.cpp
[removed]
45 assert(Thread::current()->is_Java_thread(),
46 "Expected main thread to be a JavaThread");

This was an intentional redundancy with the following JavaThread::current(),
to provide a more informative error message.

[Sorry about missing this on my first pass through.]

------------------------------------------------------------------------------
src/hotspot/share/jvmci/jvmciEnv.cpp
243 void JVMCIEnv::describe_pending_exception(bool clear) {
244 JavaThread* THREAD = JavaThread::current();

This change looks suspicious. The old code used Thread::current() here and
later cast to a JavaThread*. But that use and cast is conditional, and
might not be executed at all, depending on the result of !is_hotspot(). So
previously if !is_hotspot() then the current thread could be a non-JavaThread.

(At least locally; there could be things being called in the !is_hotspot()
case that also require a current JavaThread.)

It also looks like the THREAD variable could be eliminated and it's one use
changed to JavaThread::current().

------------------------------------------------------------------------------
src/hotspot/share/prims/jvm.cpp
992 assert(THREAD->is_Java_thread(), "must be a JavaThread");

It's not obvious why this assert is being removed.

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/concurrentGCBreakpoints.cpp

63 #define assert_Java_thread() \
64 assert(Thread::current()->is_Java_thread(), "precondition")
65
66 void ConcurrentGCBreakpoints::run_to_idle_impl(bool acquiring_control) {
67 assert_Java_thread();
68 MonitorLocker ml(monitor());

=>

63 void ConcurrentGCBreakpoints::run_to_idle_impl(bool acquiring_control) {
64 MonitorLocker ml(JavaThread::current(), monitor());

and related later changes in this file.

I prefer the original code, which intentionally made the precondition check
explicit.

[Sorry about missing this on my first pass through.]

------------------------------------------------------------------------------
src/hotspot/share/runtime/objectMonitor.cpp
1255 bool ObjectMonitor::reenter(intx recursions, TRAPS) {
1256 Thread * const Self = THREAD;
1257 JavaThread * jt = Self->as_Java_thread();

The only use of Self could just be THREAD, which is also used in this
function. And I don't see any references to jt at all here. Maybe that
should just be an `assert(THREAD->is_Java_thread(), "precondition");`.

Hm, there are other functions in this file that have a peculiar mix of
Self/THREAD usage and bound but unused jt.

Cleaning that up should probably be a separate task; this changeset is
already quite big enough!

------------------------------------------------------------------------------
src/hotspot/share/services/management.cpp
2074 if (thread_id == 0) { // current thread
2075 return thread->cooked_allocated_bytes();

Indentation got messed up.

------------------------------------------------------------------------------

It seems like there are some places where if a function parameter were
JavaThread* rather than Thread* then there wouldn't need to be a call to
as_Java_thread. That is, use the type system to make sure the types are
correct, rather than a debug-only runtime check. But that's something to
look at as a followup.

------------------------------------------------------------------------------

[mlbridge]

Mailing list message from Kim Barrett on shenandoah-dev:

On Sep 8, 2020, at 11:30 PM, Kim Barrett <kim.barrett at oracle.com> wrote:

I reviewed all of v4, not just the gc parts this time.

I guess it was v5 I was reviewing. There seems to be an inconsistency in numbering.

[mlbridge]

Mailing list message from David Holmes on shenandoah-dev:

Hi Kim,

On 9/09/2020 1:30 pm, Kim Barrett wrote:

I reviewed all of v4, not just the gc parts this time.

Thank you.

Note that I've been using the version as given in the email subject
(i.e. v5 here) not the "range" value (04 here) listed in the webrev
links. Sorry for any confusion.

------------------------------------------------------------------------------
src/hotspot/share/classfile/systemDictionary.cpp

The block from line 1646-1655 seems to be misindented. And was before too.

Fixed.

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/barrierSet.cpp
[removed]
45 assert(Thread::current()->is_Java_thread(),
46 "Expected main thread to be a JavaThread");

This was an intentional redundancy with the following JavaThread::current(),
to provide a more informative error message.

[Sorry about missing this on my first pass through.]

Reverted.

------------------------------------------------------------------------------
src/hotspot/share/jvmci/jvmciEnv.cpp
243 void JVMCIEnv::describe_pending_exception(bool clear) {
244 JavaThread* THREAD = JavaThread::current();

This change looks suspicious. The old code used Thread::current() here and
later cast to a JavaThread*. But that use and cast is conditional, and
might not be executed at all, depending on the result of !is_hotspot(). So
previously if !is_hotspot() then the current thread could be a non-JavaThread.

(At least locally; there could be things being called in the !is_hotspot()
case that also require a current JavaThread.)

Yes - if you look at JNIAccessMark it manifests JavaThread::current() by
default.

It also looks like the THREAD variable could be eliminated and it's one use
changed to JavaThread::current().

Or I can keep it and also use it for the JNIAccessMark constructor.

------------------------------------------------------------------------------
src/hotspot/share/prims/jvm.cpp
992 assert(THREAD->is_Java_thread(), "must be a JavaThread");

It's not obvious why this assert is being removed.

The fact THREAD must be a JavaThread is already established by the
JVM_ENTRY calls that then invoke this local method.

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/concurrentGCBreakpoints.cpp

63 #define assert_Java_thread() \
64 assert(Thread::current()->is_Java_thread(), "precondition")
65
66 void ConcurrentGCBreakpoints::run_to_idle_impl(bool acquiring_control) {
67 assert_Java_thread();
68 MonitorLocker ml(monitor());

=>

63 void ConcurrentGCBreakpoints::run_to_idle_impl(bool acquiring_control) {
64 MonitorLocker ml(JavaThread::current(), monitor());

and related later changes in this file.

I prefer the original code, which intentionally made the precondition check
explicit.

The same precondition is already present in the use of
JavaThread::current(). Is that not explicit enough? Plus the old code
will manifest the current thread twice in debug builds. Cleaning up this
kind of redundancy is one of the key aims here. :(

[Sorry about missing this on my first pass through.]

------------------------------------------------------------------------------
src/hotspot/share/runtime/objectMonitor.cpp
1255 bool ObjectMonitor::reenter(intx recursions, TRAPS) {
1256 Thread * const Self = THREAD;
1257 JavaThread * jt = Self->as_Java_thread();

The only use of Self could just be THREAD, which is also used in this
function. And I don't see any references to jt at all here. Maybe that
should just be an `assert(THREAD->is_Java_thread(), "precondition");`.

Hm, there are other functions in this file that have a peculiar mix of
Self/THREAD usage and bound but unused jt.

Cleaning that up should probably be a separate task; this changeset is
already quite big enough!

Right, I tried to avoid the temptation of dealing with the
Self/THREAD/jt mess in this change. I have another RFE filed that will
do further cleanup here:

https://bugs.openjdk.java.net/browse/JDK-8252685

------------------------------------------------------------------------------
src/hotspot/share/services/management.cpp
2074 if (thread_id == 0) { // current thread
2075 return thread->cooked_allocated_bytes();

Indentation got messed up.

Fixed. (I must remember to try the emacs fix to handle the JVM_ENTRY and
other macros.)

------------------------------------------------------------------------------

It seems like there are some places where if a function parameter were
JavaThread* rather than Thread* then there wouldn't need to be a call to
as_Java_thread. That is, use the type system to make sure the types are
correct, rather than a debug-only runtime check. But that's something to
look at as a followup.

Yep - that same RFE:

https://bugs.openjdk.java.net/browse/JDK-8252685

Thanks,
David
-----

------------------------------------------------------------------------------

[mlbridge]

Mailing list message from Kim Barrett on shenandoah-dev:

Hi David. Thanks for clarifying some bits I was confused abut.

src/hotspot/share/jvmci/jvmciEnv.cpp
243 void JVMCIEnv::describe_pending_exception(bool clear) {
244 JavaThread* THREAD = JavaThread::current();
This change looks suspicious. The old code used Thread::current() here and
later cast to a JavaThread*. But that use and cast is conditional, and
might not be executed at all, depending on the result of !is_hotspot(). So
previously if !is_hotspot() then the current thread could be a non-JavaThread.
(At least locally; there could be things being called in the !is_hotspot()
case that also require a current JavaThread.)

Yes - if you look at JNIAccessMark it manifests JavaThread::current() by default.

It also looks like the THREAD variable could be eliminated and it's one use
changed to JavaThread::current().

Or I can keep it and also use it for the JNIAccessMark constructor.

I see; good.

src/hotspot/share/prims/jvm.cpp
992 assert(THREAD->is_Java_thread(), "must be a JavaThread");
It's not obvious why this assert is being removed.

The fact THREAD must be a JavaThread is already established by the JVM_ENTRY calls that then invoke this local method.

Right. I keep forgetting which of these macros does what with which kinds of threads.

src/hotspot/share/gc/shared/concurrentGCBreakpoints.cpp
63 #define assert_Java_thread() \
64 assert(Thread::current()->is_Java_thread(), "precondition")
65
66 void ConcurrentGCBreakpoints::run_to_idle_impl(bool acquiring_control) {
67 assert_Java_thread();
68 MonitorLocker ml(monitor());
=>
63 void ConcurrentGCBreakpoints::run_to_idle_impl(bool acquiring_control) {
64 MonitorLocker ml(JavaThread::current(), monitor());
and related later changes in this file.
I prefer the original code, which intentionally made the precondition check
explicit.

The same precondition is already present in the use of JavaThread::current(). Is that not explicit enough? Plus the old code will manifest the current thread twice in debug builds. Cleaning up this kind of redundancy is one of the key aims here. :(

I'm not that concerned by a little redundant work in a debug build. I think
the explicit assert is clearer here. It removes the question for the future
reader of why it's asking for a JavaThread for the mutex locker, when any
thread can be used there. It's not obvious that the reason is to get the
associated assertion.

src/hotspot/share/runtime/objectMonitor.cpp
1255 bool ObjectMonitor::reenter(intx recursions, TRAPS) {
1256 Thread * const Self = THREAD;
1257 JavaThread * jt = Self->as_Java_thread();
The only use of Self could just be THREAD, which is also used in this
function. And I don't see any references to jt at all here. Maybe that
should just be an `assert(THREAD->is_Java_thread(), "precondition");`.
Hm, there are other functions in this file that have a peculiar mix of
Self/THREAD usage and bound but unused jt.
Cleaning that up should probably be a separate task; this changeset is
already quite big enough!

Right, I tried to avoid the temptation of dealing with the Self/THREAD/jt mess in this change. I have another RFE filed that will do further cleanup here:

https://bugs.openjdk.java.net/browse/JDK-8252685

Good.

[mlbridge]

Mailing list message from David Holmes on shenandoah-dev:

Trimming ...

On 9/09/2020 7:09 pm, Kim Barrett wrote:

src/hotspot/share/gc/shared/concurrentGCBreakpoints.cpp
63 #define assert_Java_thread() \
64 assert(Thread::current()->is_Java_thread(), "precondition")
65
66 void ConcurrentGCBreakpoints::run_to_idle_impl(bool acquiring_control) {
67 assert_Java_thread();
68 MonitorLocker ml(monitor());
=>
63 void ConcurrentGCBreakpoints::run_to_idle_impl(bool acquiring_control) {
64 MonitorLocker ml(JavaThread::current(), monitor());
and related later changes in this file.
I prefer the original code, which intentionally made the precondition check
explicit.

The same precondition is already present in the use of JavaThread::current(). Is that not explicit enough? Plus the old code will manifest the current thread twice in debug builds. Cleaning up this kind of redundancy is one of the key aims here. :(

I'm not that concerned by a little redundant work in a debug build. I think
the explicit assert is clearer here. It removes the question for the future
reader of why it's asking for a JavaThread for the mutex locker, when any
thread can be used there. It's not obvious that the reason is to get the
associated assertion.

I personally think that the use of JavaThread::current() makes a clear
statement that a JavaThread is expected here, but I have reverted the
change.

Thanks,
David
-----

src/hotspot/share/runtime/objectMonitor.cpp
1255 bool ObjectMonitor::reenter(intx recursions, TRAPS) {
1256 Thread * const Self = THREAD;
1257 JavaThread * jt = Self->as_Java_thread();
The only use of Self could just be THREAD, which is also used in this
function. And I don't see any references to jt at all here. Maybe that
should just be an `assert(THREAD->is_Java_thread(), "precondition");`.
Hm, there are other functions in this file that have a peculiar mix of
Self/THREAD usage and bound but unused jt.
Cleaning that up should probably be a separate task; this changeset is
already quite big enough!

Right, I tried to avoid the temptation of dealing with the Self/THREAD/jt mess in this change. I have another RFE filed that will do further cleanup here:

https://bugs.openjdk.java.net/browse/JDK-8252685

Good.

[dcubed-ojdk]

This is a really nice set of cleanup changes.

I have a few comments.

https://openjdk.github.io/cr/?repo=jdk&pr=37&range=06#frames-33
51 if (thread->is_Java_thread())
52 return thread->as_Java_thread()->thread_state() == _thread_in_vm;
53 else
54 return thread->is_VM_thread();
nit - this code needs braces. Not your bug, but since you've touched this
code, do you mind fixing it?

Note: I included the link the webrev had me on, but I have no idea what
file that is...

https://openjdk.github.io/cr/?repo=jdk&pr=37&range=06#frames-80

276 guarantee(current == get_thread() || current == get_thread()->active_handshaker(),
277 "must be current thread or direct handshake");

nit - the indent on L277 looks wrong in the webrev, but it looks right in
this paste. I don't know which is telling the truth.

https://openjdk.github.io/cr/?repo=jdk&pr=37&range=06#frames-101

358 this->as_Java_thread()->set_stack_overflow_limit();
359 this->as_Java_thread()->set_reserved_stack_activation(stack_base());
nit - do you really need 'this->' here?

https://openjdk.github.io/cr/?repo=jdk&pr=37&range=06#frames-107

old code:
2074 if (thread_id == 0) {
2075 // current thread
2076 if (THREAD->is_Java_thread()) {
2077 return ((JavaThread*)THREAD)->cooked_allocated_bytes();
2078 }
2079 return -1;

new code:
2074 if (thread_id == 0) { // current thread
2075 return thread->cooked_allocated_bytes();

If the calling thread is not a JavaThread and passes a thread_id ==0,
I don't think the returns are equivalent.

The craziness in the JavaThread::pd_get_top_frame() functions made my head hurt.
Thanks for cleaning that up!

[dcubed-ojdk]

/reviewer add dcubed_ojdk

[openjdk]

@dcubed-ojdk Only the author (@dholmes-ora) is allowed to issue the reviewer command.

[dcubed-ojdk]

I don't see how to add myself as a reviewer... what am I missing?

[mlbridge]

Mailing list message from David Holmes on shenandoah-dev:

Hi Dan,

Thanks for looking at this one.

On 10/09/2020 9:26 am, Daniel D.Daugherty wrote:

On Wed, 9 Sep 2020 14:06:02 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

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

Reverted to the original code as the explicit assertion is preferred.

Marked as reviewed by kbarrett (Reviewer).

This is a really nice set of cleanup changes.

I have a few comments.

https://openjdk.github.io/cr/?repo=jdk&pr=37&range=06#frames-33
51 if (thread->is_Java_thread())
52 return thread->as_Java_thread()->thread_state() == _thread_in_vm;
53 else
54 return thread->is_VM_thread();
nit - this code needs braces. Not your bug, but since you've touched this
code, do you mind fixing it?

Yes will add braces.

So glad you picked on this though as I messed up one of my commits and
rolled back to the v1 version, forgetting that it was broken in v1. The
original line is:

return true; //something like this: thread->is_VM_thread();

so I tried:

return thread->is_VM_thread();

but it causes the assertion to fail as GC threads also execute this. So
I've restored to the original "return true;" but updated the comment.

Note: I included the link the webrev had me on, but I have no idea what
file that is...

Yeah those links are pretty obscure, and even the webrev frames view it
takes you to doesn't give the name of the file. :(

https://openjdk.github.io/cr/?repo=jdk&pr=37&range=06#frames-80

276 guarantee(current == get_thread() || current == get_thread()->active_handshaker(),
277 "must be current thread or direct handshake");

 nit \- the indent on L277 looks wrong in the webrev\, but it looks right in
 this paste\. I don\'t know which is telling the truth\.

It was wrong - fixed.

https://openjdk.github.io/cr/?repo=jdk&pr=37&range=06#frames-101

358 this->as_Java_thread()->set_stack_overflow_limit();
359 this->as_Java_thread()->set_reserved_stack_activation(stack_base());
nit - do you really need 'this->' here?

Nope. Fixed.

https://openjdk.github.io/cr/?repo=jdk&pr=37&range=06#frames-107

old code:
2074 if (thread_id == 0) {
2075 // current thread
2076 if (THREAD->is_Java_thread()) {
2077 return ((JavaThread*)THREAD)->cooked_allocated_bytes();
2078 }
2079 return -1;

new code:
2074 if (thread_id == 0) { // current thread
2075 return thread->cooked_allocated_bytes();

If the calling thread is not a JavaThread and passes a thread_id ==0,
I don't think the returns are equivalent.

This code is in a JVM_ENTRY - so both THREAD and thread refer to
JavaThread::current(), so we can never hit the "return -1;".

The craziness in the JavaThread::pd_get_top_frame() functions made my head hurt.
Thanks for cleaning that up!

Thanks again for the review. v8 will be appearing shortly.

David

[dcubed-ojdk]

Incremental looks good to me.

[dholmes-ora]

/integrate

[openjdk]

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

• 4880226: 8171303: sun/java2d/pipe/InterpolationQualityTest.java fails on Windows & Linux
• 8da6c8d: 7183828: Invalid Image Variant when using anything other than BufferedImage
• ff21696: 8252817: Cleanup the classes in the java.awt.color package
• 44a74da: 8252919: JDK built with --enable-cds=no fails with NoClassDefFoundError
• 418e4a2: 8252830: Correct missing javadoc comments in java.rmi module
• 41d29b7: 8252774: remove jdk.test.lib.FileInstaller action from graalunit tests
• 5b30a83: 8252778: remove jdk.test.lib.FileInstaller action from compiler/c2/stemmer test
• 8db3335: 8247928: Refactor G1ConcurrentMarkThread for mark abort
• 7ccf435: 8252846: Fix ppc/s390 after "8231441: AArch64: Initial SVE backend su…
• d236cf4: 8252995: Non-PCH builds broken by JDK-8250961
• ... and 34 more: https://git.openjdk.java.net/jdk/compare/5f76deb2e064ca8a48fb8a638c23aad34bf27f9c...master

Your commit was automatically rebased without conflicts.

Pushed as commit 976acdd.

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