RFR: 8264711: More runtime TRAPS cleanups

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

RFR: 8264711: More runtime TRAPS cleanups

Harold Seigel-2
Please review this additional cleanup of use of TRAPS in hotspot runtime code.  The changes were tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows and Mach5 tiers 3-5 on Linux x64.

Thanks, Harold

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

Commit messages:
 - 8264711: More runtime TRAPS cleanups

Changes: https://git.openjdk.java.net/jdk/pull/3345/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3345&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264711
  Stats: 146 lines in 32 files changed: 5 ins; 19 del; 122 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3345.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3345/head:pull/3345

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

Re: RFR: 8264711: More runtime TRAPS cleanups

Patricio Chilano Mateo
On Mon, 5 Apr 2021 17:57:13 GMT, Harold Seigel <[hidden email]> wrote:

> Please review this additional cleanup of use of TRAPS in hotspot runtime code.  The changes were tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows and Mach5 tiers 3-5 on Linux x64.
>
> Thanks, Harold

Hi Harold,

I looked at the changes to synchronization code and looks good except for one issue below.

Thanks,
Patricio

src/hotspot/share/prims/jni.cpp line 2738:

> 2736:
> 2737:   Handle obj(THREAD, JNIHandles::resolve_non_null(jobj));
> 2738:   ObjectSynchronizer::jni_exit(THREAD->as_Java_thread(), obj());

Here we would return JNI_ERR if we throw IMSE from jni_exit().

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

> 607:   // intentionally do not use CHECK on check_owner because we must exit the
> 608:   // monitor even if an exception was already pending.
> 609:   if (monitor->check_owner(current)) {

We can actually throw IMSE from check_owner() if this thread is not the real owner.

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

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

Re: RFR: 8264711: More runtime TRAPS cleanups [v2]

Harold Seigel-2
In reply to this post by Harold Seigel-2
> Please review this additional cleanup of use of TRAPS in hotspot runtime code.  The changes were tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows and Mach5 tiers 3-5 on Linux x64.
>
> Thanks, Harold

Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:

  Undo change to ObjectSynchronizer::jni_exit()

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3345/files
  - new: https://git.openjdk.java.net/jdk/pull/3345/files/b83f1404..e488fb8a

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

  Stats: 6 lines in 4 files changed: 1 ins; 0 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3345.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3345/head:pull/3345

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

Re: RFR: 8264711: More runtime TRAPS cleanups [v2]

Harold Seigel-2
In reply to this post by Patricio Chilano Mateo
On Mon, 5 Apr 2021 19:11:49 GMT, Patricio Chilano Mateo <[hidden email]> wrote:

>> Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Undo change to ObjectSynchronizer::jni_exit()
>
> Hi Harold,
>
> I looked at the changes to synchronization code and looks good except for one issue below.
>
> Thanks,
> Patricio

Thanks Lois and Patricio for reviewing the change!
I removed my bogus change to ObjectSynchronizer::jni_exit() which also made calls consistent in jfrJavaSupport.cpp.
Harold

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

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

Re: RFR: 8264711: More runtime TRAPS cleanups [v2]

Patricio Chilano Mateo
In reply to this post by Harold Seigel-2
On Mon, 5 Apr 2021 20:27:53 GMT, Harold Seigel <[hidden email]> wrote:

>> Please review this additional cleanup of use of TRAPS in hotspot runtime code.  The changes were tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows and Mach5 tiers 3-5 on Linux x64.
>>
>> Thanks, Harold
>
> Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:
>
>   Undo change to ObjectSynchronizer::jni_exit()

Marked as reviewed by pchilanomate (Committer).

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

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

Re: RFR: 8264711: More runtime TRAPS cleanups [v2]

Patricio Chilano Mateo
In reply to this post by Harold Seigel-2
On Mon, 5 Apr 2021 20:24:31 GMT, Harold Seigel <[hidden email]> wrote:

> Thanks Lois and Patricio for reviewing the change!
> I removed my bogus change to ObjectSynchronizer::jni_exit() which also made calls consistent in jfrJavaSupport.cpp.
> Harold
Thanks Harold! Fix looks good.

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

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

Re: RFR: 8264711: More runtime TRAPS cleanups [v2]

David Holmes-2
In reply to this post by Harold Seigel-2
On Mon, 5 Apr 2021 20:27:53 GMT, Harold Seigel <[hidden email]> wrote:

>> Please review this additional cleanup of use of TRAPS in hotspot runtime code.  The changes were tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows and Mach5 tiers 3-5 on Linux x64.
>>
>> Thanks, Harold
>
> Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:
>
>   Undo change to ObjectSynchronizer::jni_exit()

Hi Harold,

Lots of good cleanup here but also a couple of things that I think are incorrect. Platform string creation can throw exceptions; and I also believe the ClassFileLoadHook can too.

Thanks,
David

src/hotspot/share/classfile/javaClasses.cpp line 396:

> 394:
> 395: // Converts a C string to a Java String based on current encoding
> 396: Handle java_lang_String::create_from_platform_dependent_str(JavaThread* current, const char* str) {

This change is incorrect. JNU_NewStringPlatform can raise an exception so TRAPS here is correct.

src/hotspot/share/classfile/klassFactory.cpp line 117:

> 115:                                                    Handle protection_domain,
> 116:                                                    JvmtiCachedClassFileData** cached_class_file,
> 117:                                                    TRAPS) {

I don't think this removal of TRAPS is correct. The  ClassFileLoadHook could result in an exception becoming pending.

src/hotspot/share/prims/jvmtiEnv.cpp line 709:

> 707:
> 708:     // need the path as java.lang.String
> 709:     Handle path = java_lang_String::create_from_platform_dependent_str(THREAD, segment);

This change will need reverting as an exception is possible as previously noted.

But note that if there was no possibility of exception here then the following check of HAS_PENDING_EXCEPTION should also have been removed.

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

Changes requested by dholmes (Reviewer).

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

Re: RFR: 8264711: More runtime TRAPS cleanups [v4]

Harold Seigel-2
In reply to this post by Harold Seigel-2
> Please review this additional cleanup of use of TRAPS in hotspot runtime code.  The changes were tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows and Mach5 tiers 3-5 on Linux x64.
>
> Thanks, Harold

Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:

  remove unneeded statement

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3345/files
  - new: https://git.openjdk.java.net/jdk/pull/3345/files/1ee327f0..0f1b4f6b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3345&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3345&range=02-03

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

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

Re: RFR: 8264711: More runtime TRAPS cleanups [v4]

Harold Seigel-2
In reply to this post by David Holmes-2
On Mon, 5 Apr 2021 23:15:48 GMT, David Holmes <[hidden email]> wrote:

>> Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   remove unneeded statement
>
> src/hotspot/share/classfile/klassFactory.cpp line 117:
>
>> 115:                                                    ClassLoaderData* loader_data,
>> 116:                                                    Handle protection_domain,
>> 117:                                                    JvmtiCachedClassFileData** cached_class_file) {
>
> I don't think this removal of TRAPS is correct. The  ClassFileLoadHook could result in an exception becoming pending.

Thanks!  This change was reverted.

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

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

Re: RFR: 8264711: More runtime TRAPS cleanups [v4]

Daniel D.Daugherty
In reply to this post by Harold Seigel-2
On Wed, 7 Apr 2021 14:19:50 GMT, Harold Seigel <[hidden email]> wrote:

>> Please review this additional cleanup of use of TRAPS in hotspot runtime code.  The changes were tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows and Mach5 tiers 3-5 on Linux x64.
>>
>> Thanks, Harold
>
> Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:
>
>   remove unneeded statement

Marked as reviewed by dcubed (Reviewer).

src/hotspot/share/interpreter/bytecode.cpp line 160:

> 158: }
> 159:
> 160: Handle Bytecode_invoke::appendix(TRAPS) {

You might want to mention the removal of this function in
the bug report so that it will show up in a JBS search.

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

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

Re: RFR: 8264711: More runtime TRAPS cleanups [v4]

Daniel D.Daugherty
On Wed, 7 Apr 2021 15:40:58 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   remove unneeded statement
>
> Marked as reviewed by dcubed (Reviewer).

Thumbs up.

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

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

Integrated: 8264711: More runtime TRAPS cleanups

Harold Seigel-2
In reply to this post by Harold Seigel-2
On Mon, 5 Apr 2021 17:57:13 GMT, Harold Seigel <[hidden email]> wrote:

> Please review this additional cleanup of use of TRAPS in hotspot runtime code.  The changes were tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows and Mach5 tiers 3-5 on Linux x64.
>
> Thanks, Harold

This pull request has now been integrated.

Changeset: af13c64f
Author:    Harold Seigel <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/af13c64f
Stats:     97 lines in 26 files changed: 1 ins; 13 del; 83 mod

8264711: More runtime TRAPS cleanups

Reviewed-by: lfoltan, pchilanomate, dholmes, dcubed

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

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