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

8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines #3062

Closed
wants to merge 1 commit into from

Conversation

@dholmes-ora
Copy link
Member

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

All JRT_ENTRY routines are required to have a parameter, "JavaThread* thread", which must be the current thread. The JRT_ENTRY, and related macros, then use this parameter and also expose it as THREAD for use with exception macros. But the fact "thread" is the current thread is lost in some routines and we see strange calls to other code that pass both "thread" and "THREAD" as distinct parameters - primarily when a TRAPS method is involved.

This should be cleaned up along with a general check on misuse of TRAPS/THREAD.

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-8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

Download

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

To update a local copy of the PR:
$ git checkout pull/3062
$ git pull https://git.openjdk.java.net/jdk pull/3062/head

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 18, 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 openjdk bot commented Mar 18, 2021

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

  • hotspot

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.

@openjdk openjdk bot added the hotspot label Mar 18, 2021
@dholmes-ora
Copy link
Member Author

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

/label remove hotspot
/label add hotspot-runtime

@openjdk openjdk bot removed the hotspot label Mar 18, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 18, 2021

@dholmes-ora
The hotspot label was successfully removed.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 18, 2021

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

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

@mlbridge mlbridge bot commented Mar 18, 2021

Webrevs

Copy link

@coleenp coleenp left a comment

Can you state the convention that you're trying to establish?
I would like it to be when there is TRAPS, use THREAD for Handles, otherwise use current if passed as JavaThread* current.

@@ -1212,23 +1214,21 @@ methodHandle SharedRuntime::find_callee_method(JavaThread* thread, TRAPS) {
fr = fr.sender(&reg_map);
assert(fr.is_entry_frame(), "must be");
// fr is now pointing to the entry frame.
callee_method = methodHandle(THREAD, fr.entry_frame_call_wrapper()->callee_method());
callee_method = methodHandle(thread, fr.entry_frame_call_wrapper()->callee_method());

This comment has been minimized.

@coleenp

coleenp Mar 18, 2021

Ok, now this just seems arbitrary to me. I don't see why we should introduce a JavaThread* thread, when there's a perfectly good THREAD available here, and THREAD is fine here. There's a lot of code that uses THREAD as a Handle parameter and there's nothing wrong with it. If it's lower case 'thread', one has to wonder and look around to see if it's the current thread (changing the convention to 'current' was good where you did that). Don't change these ones. It seems like a conflict of your preferences to what's in the code, and I vote for what's used in the code everywhere. Removing TRAPS from functions that don't throw an exception is also good. But I don't see the point of this at all and I think it's worse.

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 18, 2021
Author Member

The intent is that THREAD, ideally, is only used in relation to exception throwing/catching and you can't tell that if you use it just for convenience everywhere else. I haven't introduced a new local "thread" when there is only a single use, but if there are multiple uses then using "thread" or "current" is IMO much clearer because we can clearly isolate the the code that relates to exceptions and the code that does not. That is what I'm trying to achieve here. It wont be perfect and not every use of THREAD will be replaced, but it will be a great improvement over what we currently have.
In this code I would love to change "thread" to "current" but the JRT macros require the use of "thread" and changing that would be too big a change. Note that in the code above I'm not introducing "thread" it is already there.

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 18, 2021
Author Member

Correction, sorry I was misled by the mis-quoted code in the discussion view, I do introduce the local here because there are multiple none-exception-related uses.

This comment has been minimized.

@coleenp

coleenp Mar 19, 2021

I don't find passing "thread" cleaner than THREAD in these cases. We know THREAD is the current thread and these functions pass it as the leading parameter is not confusing at all. These patterns: of Handle(THREAD, obj), MutexLocker ml(THREAD, lock), and methodHandle method(THREAD, m); appear everywhere. I put many of them there to pass the current thread parameter. Changing these patterns is a huge amount of churn for no benefit to me at least. I don't think I want to hunt around for a 'thread' that I hope is the current thread. I will use THREAD when needed.
Most of these changes seem harmless but I object to introducing a 'thread' just to avoid passing THREAD.

bool is_optimized, TRAPS) {

methodHandle SharedRuntime::resolve_sub_helper(bool is_virtual, bool is_optimized, TRAPS) {
JavaThread* thread = THREAD->as_Java_thread();

This comment has been minimized.

@coleenp

coleenp Mar 18, 2021

Same comment as above.

@@ -2121,7 +2121,7 @@ void SharedRuntime::monitor_enter_helper(oopDesc* obj, BasicLock* lock, JavaThre
if (PrintBiasedLockingStatistics) {
Atomic::inc(BiasedLocking::slow_path_entry_count_addr());
}
Handle h_obj(THREAD, obj);
Handle h_obj(thread, obj);

This comment has been minimized.

@coleenp

coleenp Mar 18, 2021

Can you make this one 'current'?

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 18, 2021
Author Member

Unfortunately not - the JRT_BLOCK_NO_ASYNC requires "thread".

This comment has been minimized.

@coleenp

coleenp Mar 19, 2021

Then keep it THREAD. That implies current thread.

Copy link
Member

@iklam iklam left a comment

Looks good to me in general. I have some comments about coding style.

if (trap_method.not_null()) {
MethodData* trap_mdo = trap_method->method_data();
if (trap_mdo == NULL) {
ExceptionMark em(current);
JavaThread* THREAD = current; // for exception macros

This comment has been minimized.

@iklam

iklam Mar 18, 2021
Member

Need to assert that current is indeed the current thread. Maybe we should have an inline function like this which does the current thread check?

JavaThread* THREAD = current->for_exception_handling();

(Same comment for all the other Thread* THREAD = thread; in this PR).

@@ -969,17 +971,17 @@ JRT_END
// Miscellaneous


nmethod* InterpreterRuntime::frequency_counter_overflow(JavaThread* thread, address branch_bcp) {
nmethod* InterpreterRuntime::frequency_counter_overflow(JavaThread* current, address branch_bcp) {

This comment has been minimized.

@iklam

iklam Mar 18, 2021
Member

Need to assert that current is indeed current.

(Same comment for all other parameters that you've changed to current).

@@ -1112,7 +1114,7 @@ JRT_ENTRY(void, InterpreterRuntime::update_mdp_for_ret(JavaThread* thread, int r
JRT_END

JRT_ENTRY(MethodCounters*, InterpreterRuntime::build_method_counters(JavaThread* thread, Method* m))
MethodCounters* mcs = Method::build_method_counters(m, thread);
MethodCounters* mcs = Method::build_method_counters(m, THREAD);

This comment has been minimized.

@iklam

iklam Mar 18, 2021
Member

Need ExceptionMark em(THREAD);

@@ -576,7 +576,7 @@ MethodCounters* Method::build_method_counters(Method* m, TRAPS) {
}

if (LogTouchedMethods) {
mh->log_touched(CHECK_NULL);
mh->log_touched(THREAD);

This comment has been minimized.

@iklam

iklam Mar 18, 2021
Member

When you have THREAD as the last parameter, it's not clear whether

  • You are calling a non-throwing function that just wants a thread, you
  • You are calling an exception-throwing function but you are ignoring the exception on purpose

It seems the former is the case. I would prefer something like this, which makes it clear what you're doing:

mh->log_touched(THREAD->as_JavaThread());
methodHandle callee_method;
callee_method = resolve_sub_helper(thread, is_virtual, is_optimized, THREAD);
callee_method = resolve_sub_helper(is_virtual, is_optimized, THREAD);

This comment has been minimized.

@iklam

iklam Mar 18, 2021
Member

The two occurrences of THREAD in this function seem fishy. Maybe they can be replaced with CHECK? But it's unrelated to this PR so probably should be done in a separate REF.

This comment has been minimized.

@coleenp

coleenp Mar 19, 2021

This seems like a case where you should have removed the trailing THREAD and not the leading 'thread'.

@@ -1659,14 +1657,15 @@ bool SharedRuntime::handle_ic_miss_helper_internal(Handle receiver, CompiledMeth
return true;
}

methodHandle SharedRuntime::handle_ic_miss_helper(JavaThread *thread, TRAPS) {
methodHandle SharedRuntime::handle_ic_miss_helper(TRAPS) {
JavaThread* thread = THREAD->as_Java_thread();

This comment has been minimized.

@iklam

iklam Mar 18, 2021
Member

A general comment about the functions that took both thread and TRAPS, where you removed the thread:

Are we sure they cannot be called on a non-current thread?

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 18, 2021
Author Member

Everything goes back to the JRT macros where THREAD is created from thread, so they are always the same current thread instance.

This comment has been minimized.

@iklam

iklam Mar 19, 2021
Member

Have you check all possible call paths? Could one of these functions be called from a place other than the JRT functions?

This comment has been minimized.

@coleenp

coleenp Mar 19, 2021

This is why THREAD is clearly the current thread because that's what the JRT_/JVM_ macros do. Just use THREAD as the current thread for ResourceMark and whatever else needs it below.

methodHandle callee_method;
callee_method = resolve_sub_helper(thread, is_virtual, is_optimized, THREAD);
callee_method = resolve_sub_helper(is_virtual, is_optimized, THREAD);

This comment has been minimized.

@coleenp

coleenp Mar 19, 2021

This seems like a case where you should have removed the trailing THREAD and not the leading 'thread'.

@@ -1659,14 +1657,15 @@ bool SharedRuntime::handle_ic_miss_helper_internal(Handle receiver, CompiledMeth
return true;
}

methodHandle SharedRuntime::handle_ic_miss_helper(JavaThread *thread, TRAPS) {
methodHandle SharedRuntime::handle_ic_miss_helper(TRAPS) {
JavaThread* thread = THREAD->as_Java_thread();

This comment has been minimized.

@coleenp

coleenp Mar 19, 2021

This is why THREAD is clearly the current thread because that's what the JRT_/JVM_ macros do. Just use THREAD as the current thread for ResourceMark and whatever else needs it below.

@@ -1766,7 +1765,8 @@ static bool clear_ic_at_addr(CompiledMethod* caller_nm, address call_addr, bool
// sites, and static call sites. Typically used to change a call sites
// destination from compiled to interpreted.
//
methodHandle SharedRuntime::reresolve_call_site(JavaThread *thread, TRAPS) {
methodHandle SharedRuntime::reresolve_call_site(TRAPS) {
JavaThread* thread = THREAD->as_Java_thread();
ResourceMark rm(thread);
RegisterMap reg_map(thread, false);
frame stub_frame = thread->last_frame();

This comment has been minimized.

@coleenp

coleenp Mar 19, 2021

The call to thread->last_frame() may require JavaThread. I think it's a good change to remove the thread parameter.

@@ -2121,7 +2121,7 @@ void SharedRuntime::monitor_enter_helper(oopDesc* obj, BasicLock* lock, JavaThre
if (PrintBiasedLockingStatistics) {
Atomic::inc(BiasedLocking::slow_path_entry_count_addr());
}
Handle h_obj(THREAD, obj);
Handle h_obj(thread, obj);

This comment has been minimized.

@coleenp

coleenp Mar 19, 2021

Then keep it THREAD. That implies current thread.

@coleenp
Copy link

@coleenp coleenp commented Mar 19, 2021

I thought the purpose of this work was to make TRAPS and the JRT/JVM_ENTRY macros take JavaThread instead of Thread.
ie #define TRAPS JavaThread* THREAD

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 21, 2021

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

On 19/03/2021 10:00 pm, Coleen Phillimore wrote:

On Fri, 19 Mar 2021 01:48:47 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

All JRT_ENTRY routines are required to have a parameter, "JavaThread* thread", which must be the current thread. The JRT_ENTRY, and related macros, then use this parameter and also expose it as THREAD for use with exception macros. But the fact "thread" is the current thread is lost in some routines and we see strange calls to other code that pass both "thread" and "THREAD" as distinct parameters - primarily when a TRAPS method is involved.

This should be cleaned up along with a general check on misuse of TRAPS/THREAD.

Testing: tiers 1-3

Thanks,
David

Changes requested by coleenp (Reviewer).

I thought the purpose of this work was to make TRAPS and the JRT/JVM_ENTRY macros take JavaThread instead of Thread.
ie #define TRAPS JavaThread* THREAD

That is the endgame for JDK-8252685. The current issue is a step along
the way to make that one more digestible. The entry macros already work
with the current JavaThread:
- JRT_ENTRY uses the "thread" parameter of the wrapped method
- JVM_ENTRY and JNI_ENTRY extract it from the JNIEnv

and in all cases they then declare an alias:

Thread* THREAD = thread;

for use with TRAPS/CHECK

David
-----

@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented Apr 8, 2021

I'm closing this PR and coming back with a new version that we have discussed offline. A clean start will be easier.

@dholmes-ora dholmes-ora closed this Apr 8, 2021
@dholmes-ora dholmes-ora deleted the dholmes-ora:8263709 branch Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants