Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines [v2]

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines [v2]

David Holmes-2
> The existing JRT_ENTRY (and related) macros require the function to which they are applied to declare a parameter "JavaThread* thread" which represents the current thread. These functions are all implicitly "traps" functions as they can result in exceptions, but they are not declared with TRAPS because the only caller of these functions is the runtime itself (via call_VM) and no callers need to be aware to use CHECK; further they need a JavaThread. So the macro declares the THREAD variable for use with other exception-producing functions and assigns it from "thread".
>
> The majority of this change replaces the parameter name "thread" with "current" so that it is clear that we are always dealing with the current thread. This affects the entry functions as well as the functions called therefrom.
>
> We can then also replace the use of "THREAD" with "current", in contexts that are not related to exception processing.
>
> Some methods called by entry functions were declared to have both a "thread" parameter and a "TRAPS" parameter - with nothing to tell you these are always the same, current, thread. So the "thread" parameter is removed and replaced with a local variable "current" obtained from THREAD->as_Java_thread().
>
> Some missing CHECK_ uses were added.
>
> Testing:
>   - tiers 1-3
>
> Thanks,
> David

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

  Fixed CHECK on return statement.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3394/files
  - new: https://git.openjdk.java.net/jdk/pull/3394/files/78298e5f..1adf0fd0

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3394&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3394&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3394.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3394/head:pull/3394

PR: https://git.openjdk.java.net/jdk/pull/3394
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines [v2]

Coleen Phillimore-3
On Thu, 8 Apr 2021 09:25:48 GMT, David Holmes <[hidden email]> wrote:

>> The existing JRT_ENTRY (and related) macros require the function to which they are applied to declare a parameter "JavaThread* thread" which represents the current thread. These functions are all implicitly "traps" functions as they can result in exceptions, but they are not declared with TRAPS because the only caller of these functions is the runtime itself (via call_VM) and no callers need to be aware to use CHECK; further they need a JavaThread. So the macro declares the THREAD variable for use with other exception-producing functions and assigns it from "thread".
>>
>> The majority of this change replaces the parameter name "thread" with "current" so that it is clear that we are always dealing with the current thread. This affects the entry functions as well as the functions called therefrom.
>>
>> We can then also replace the use of "THREAD" with "current", in contexts that are not related to exception processing.
>>
>> Some methods called by entry functions were declared to have both a "thread" parameter and a "TRAPS" parameter - with nothing to tell you these are always the same, current, thread. So the "thread" parameter is removed and replaced with a local variable "current" obtained from THREAD->as_Java_thread().
>>
>> Some missing CHECK_ uses were added.
>>
>> Testing:
>>   - tiers 1-3
>>
>> Thanks,
>> David
>
> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fixed CHECK on return statement.

I think substituting "JavaThread* thread" for "JavaThread* current" is a good change and convention that makes the code more clear, so worth the dull code review and diffs.

src/hotspot/share/c1/c1_Runtime1.cpp line 180:

> 178: static void deopt_caller() {
> 179:   if ( !caller_is_deopted()) {
> 180:     JavaThread* current = JavaThread::current();

It looks like both of these functions could be passed JavaThread from the callers.  You could leave this as a cleanup though.  It would eliminate two explicit JavaThread::current calls.

src/hotspot/share/c1/c1_Runtime1.cpp line 414:

> 412:   assert(klass->is_klass(), "not a class");
> 413:   assert(rank >= 1, "rank must be nonzero");
> 414:   Handle holder(current, klass->klass_holder()); // keep the klass alive

I think this is ok now, since current is obviously the current thread.  There doesn't seem to be a need to use THREAD here.  I don't know about changing all the uses of THREAD to current for Handles/methodHandles/ResourceMark/HandleMark but this seems ok in this context since JavaThread* current is present.

src/hotspot/share/c1/c1_Runtime1.cpp line 695:

> 693:   NOT_PRODUCT(_throw_class_cast_exception_count++;)
> 694:   ResourceMark rm(current);
> 695: char* message = SharedRuntime::generate_class_cast_message(current, object->klass());

Is this indentation off?

src/hotspot/share/interpreter/interpreterRuntime.cpp line 1157:

> 1155: JRT_END
> 1156:
> 1157: JRT_ENTRY(void, InterpreterRuntime::post_field_access(JavaThread *current, oopDesc* obj,

nit: JavaThread* current

src/hotspot/share/interpreter/interpreterRuntime.cpp line 1237:

> 1235: JRT_END
> 1236:
> 1237: JRT_ENTRY(void, InterpreterRuntime::post_method_entry(JavaThread *current))

Also move the star here and one below.

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

Marked as reviewed by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3394
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines [v2]

Harold Seigel-2
In reply to this post by David Holmes-2
On Thu, 8 Apr 2021 09:25:48 GMT, David Holmes <[hidden email]> wrote:

>> The existing JRT_ENTRY (and related) macros require the function to which they are applied to declare a parameter "JavaThread* thread" which represents the current thread. These functions are all implicitly "traps" functions as they can result in exceptions, but they are not declared with TRAPS because the only caller of these functions is the runtime itself (via call_VM) and no callers need to be aware to use CHECK; further they need a JavaThread. So the macro declares the THREAD variable for use with other exception-producing functions and assigns it from "thread".
>>
>> The majority of this change replaces the parameter name "thread" with "current" so that it is clear that we are always dealing with the current thread. This affects the entry functions as well as the functions called therefrom.
>>
>> We can then also replace the use of "THREAD" with "current", in contexts that are not related to exception processing.
>>
>> Some methods called by entry functions were declared to have both a "thread" parameter and a "TRAPS" parameter - with nothing to tell you these are always the same, current, thread. So the "thread" parameter is removed and replaced with a local variable "current" obtained from THREAD->as_Java_thread().
>>
>> Some missing CHECK_ uses were added.
>>
>> Testing:
>>   - tiers 1-3
>>
>> Thanks,
>> David
>
> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fixed CHECK on return statement.

src/hotspot/share/interpreter/interpreterRuntime.cpp line 303:

> 301:   // We'd expect to assert that we're only here to quicken bytecodes, but in a multithreaded
> 302:   // program we might have seen an unquick'd bytecode in the interpreter but have another
> 303:   // current quicken the bytecode before we get here.

This should still say 'thread', not 'current'

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

PR: https://git.openjdk.java.net/jdk/pull/3394
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines [v2]

David Holmes-2
In reply to this post by Coleen Phillimore-3
On Thu, 8 Apr 2021 12:18:12 GMT, Coleen Phillimore <[hidden email]> wrote:

>> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fixed CHECK on return statement.
>
> src/hotspot/share/c1/c1_Runtime1.cpp line 180:
>
>> 178: static void deopt_caller() {
>> 179:   if ( !caller_is_deopted()) {
>> 180:     JavaThread* current = JavaThread::current();
>
> It looks like both of these functions could be passed JavaThread from the callers.  You could leave this as a cleanup though.  It would eliminate two explicit JavaThread::current calls.

That's a good additional cleanup - thanks. Fixed.

> src/hotspot/share/c1/c1_Runtime1.cpp line 695:
>
>> 693:   NOT_PRODUCT(_throw_class_cast_exception_count++;)
>> 694:   ResourceMark rm(current);
>> 695: char* message = SharedRuntime::generate_class_cast_message(current, object->klass());
>
> Is this indentation off?

Fixed. (My emacs can't figure out how to indent when these macros are used. :( )

> src/hotspot/share/interpreter/interpreterRuntime.cpp line 1157:
>
>> 1155: JRT_END
>> 1156:
>> 1157: JRT_ENTRY(void, InterpreterRuntime::post_field_access(JavaThread *current, oopDesc* obj,
>
> nit: JavaThread* current

Fixed

> src/hotspot/share/interpreter/interpreterRuntime.cpp line 1237:
>
>> 1235: JRT_END
>> 1236:
>> 1237: JRT_ENTRY(void, InterpreterRuntime::post_method_entry(JavaThread *current))
>
> Also move the star here and one below.

Fixed all.

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

PR: https://git.openjdk.java.net/jdk/pull/3394
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines [v2]

David Holmes-2
In reply to this post by Harold Seigel-2
On Thu, 8 Apr 2021 13:11:36 GMT, Harold Seigel <[hidden email]> wrote:

>> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fixed CHECK on return statement.
>
> src/hotspot/share/interpreter/interpreterRuntime.cpp line 303:
>
>> 301:   // We'd expect to assert that we're only here to quicken bytecodes, but in a multithreaded
>> 302:   // program we might have seen an unquick'd bytecode in the interpreter but have another
>> 303:   // current quicken the bytecode before we get here.
>
> This should still say 'thread', not 'current'

Well spotted! Fixed.

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

PR: https://git.openjdk.java.net/jdk/pull/3394