RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions

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

RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions

Harold Seigel-2
Please review this small change to remove unneeded TRAPS and THREAD parameters from functions in verifier related files.  This change was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and Windows, and tiers 3-5 on Linux x64.

Thanks, Harold

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

Commit messages:
 - 8264142: Remove TRAPS/THREAD parameters for verifier related functions

Changes: https://git.openjdk.java.net/jdk/pull/3194/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3194&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264142
  Stats: 51 lines in 8 files changed: 3 ins; 9 del; 39 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3194.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3194/head:pull/3194

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions

Gerard Ziemski-2
On Thu, 25 Mar 2021 14:03:50 GMT, Harold Seigel <[hidden email]> wrote:

> Please review this small change to remove unneeded TRAPS and THREAD parameters from functions in verifier related files.  This change was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and Windows, and tiers 3-5 on Linux x64.
>
> Thanks, Harold

Looks good to me.

How did you find these places in the first places? Is there more places possible with this kind of a cleanup?

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

Marked as reviewed by gziemski (Committer).

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions

Coleen Phillimore-3
In reply to this post by Harold Seigel-2
On Thu, 25 Mar 2021 14:03:50 GMT, Harold Seigel <[hidden email]> wrote:

> Please review this small change to remove unneeded TRAPS and THREAD parameters from functions in verifier related files.  This change was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and Windows, and tiers 3-5 on Linux x64.
>
> Thanks, Harold

Changes requested by coleenp (Reviewer).

src/hotspot/share/classfile/classFileParser.cpp line 5636:

> 5634: // its _class_name field.
> 5635: void ClassFileParser::prepend_host_package_name(const InstanceKlass* unsafe_anonymous_host) {
> 5636:   JavaThread* current = JavaThread::current();

Can you pass Thread* current as the first parameter instead?  We're trying to minimize materializing the current thread.

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

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions

Harold Seigel-2
On Thu, 25 Mar 2021 17:54:53 GMT, Coleen Phillimore <[hidden email]> wrote:

>> Please review this small change to remove unneeded TRAPS and THREAD parameters from functions in verifier related files.  This change was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and Windows, and tiers 3-5 on Linux x64.
>>
>> Thanks, Harold
>
> src/hotspot/share/classfile/classFileParser.cpp line 5636:
>
>> 5634: // its _class_name field.
>> 5635: void ClassFileParser::prepend_host_package_name(const InstanceKlass* unsafe_anonymous_host) {
>> 5636:   JavaThread* current = JavaThread::current();
>
> Can you pass Thread* current as the first parameter instead?  We're trying to minimize materializing the current thread.

How about if "ResourceMark rm(THREAD)" is changed to "ResourceMark rm;" ?  Then no Thread* is need in the function.

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

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions

Coleen Phillimore-3
In reply to this post by Harold Seigel-2
On Thu, 25 Mar 2021 14:03:50 GMT, Harold Seigel <[hidden email]> wrote:

> Please review this small change to remove unneeded TRAPS and THREAD parameters from functions in verifier related files.  This change was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and Windows, and tiers 3-5 on Linux x64.
>
> Thanks, Harold

Changes requested by coleenp (Reviewer).

src/hotspot/share/classfile/verifier.cpp line 139:

> 137: void Verifier::log_end_verification(outputStream* st, const char* klassName, Symbol* exception_name) {
> 138:    Thread* THREAD = Thread::current();
> 139:

This one would be better if you passed THREAD->pending_exception() as an oop.  Then you wouldn't have to have HAS_PENDING_EXCEPTION.

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

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions

Coleen Phillimore-3
In reply to this post by Harold Seigel-2
On Thu, 25 Mar 2021 18:00:02 GMT, Harold Seigel <[hidden email]> wrote:

>> src/hotspot/share/classfile/classFileParser.cpp line 5636:
>>
>>> 5634: // its _class_name field.
>>> 5635: void ClassFileParser::prepend_host_package_name(const InstanceKlass* unsafe_anonymous_host) {
>>> 5636:   JavaThread* current = JavaThread::current();
>>
>> Can you pass Thread* current as the first parameter instead?  We're trying to minimize materializing the current thread.
>
> How about if "ResourceMark rm(THREAD)" is changed to "ResourceMark rm;" ?  Then no Thread* is need in the function.

This seems fine to me.

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

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
On Thu, 25 Mar 2021 19:15:17 GMT, Coleen Phillimore <[hidden email]> wrote:

>> Please review this small change to remove unneeded TRAPS and THREAD parameters from functions in verifier related files.  This change was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and Windows, and tiers 3-5 on Linux x64.
>>
>> Thanks, Harold
>
> src/hotspot/share/classfile/verifier.cpp line 139:
>
>> 137: void Verifier::log_end_verification(outputStream* st, const char* klassName, Symbol* exception_name) {
>> 138:    Thread* THREAD = Thread::current();
>> 139:
>
> This one would be better if you passed THREAD->pending_exception() as an oop.  Then you wouldn't have to have HAS_PENDING_EXCEPTION.

This is logging so I guess it doesn't really matter.

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

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions [v2]

Harold Seigel-2
In reply to this post by Harold Seigel-2
> Please review this small change to remove unneeded TRAPS and THREAD parameters from functions in verifier related files.  This change was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and Windows, and 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 ResourceMark use of 'THREAD'

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3194/files
  - new: https://git.openjdk.java.net/jdk/pull/3194/files/89d93d83..c96ff85a

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

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

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions

Ioi Lam
In reply to this post by Harold Seigel-2


On 3/25/21 11:02 AM, Harold Seigel wrote:
> On Thu, 25 Mar 2021 17:54:53 GMT, Coleen Phillimore <[hidden email]> wrote:
>
>>
>>> 5634: // its _class_name field.
>>> 5635: void ClassFileParser::prepend_host_package_name(const InstanceKlass* unsafe_anonymous_host) {
>>> 5636:   JavaThread* current = JavaThread::current();
>> Can you pass Thread* current as the first parameter instead?  We're trying to minimize materializing the current thread.
> How about if "ResourceMark rm(THREAD)" is changed to "ResourceMark rm;" ?  Then no Thread* is need in the function.

But we have:

   ResourceMark() : ResourceMark(Thread::current()) {}

So if the goal is to avoid calling Thread::current(), we do need to pass
in the thread.

Thanks
- Ioi
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions [v3]

Harold Seigel-2
In reply to this post by Coleen Phillimore-3
On Thu, 25 Mar 2021 19:17:05 GMT, Coleen Phillimore <[hidden email]> wrote:

>> src/hotspot/share/classfile/verifier.cpp line 139:
>>
>>> 137: void Verifier::log_end_verification(outputStream* st, const char* klassName, Symbol* exception_name) {
>>> 138:    Thread* THREAD = Thread::current();
>>> 139:
>>
>> This one would be better if you passed THREAD->pending_exception() as an oop.  Then you wouldn't have to have HAS_PENDING_EXCEPTION.
>
> This is logging so I guess it doesn't really matter.

Changed to passing it as an oop.

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

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions [v3]

Harold Seigel-2
In reply to this post by Harold Seigel-2
> Please review this small change to remove unneeded TRAPS and THREAD parameters from functions in verifier related files.  This change was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and Windows, and tiers 3-5 on Linux x64.
>
> Thanks, Harold

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

  review comment changes

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3194/files
  - new: https://git.openjdk.java.net/jdk/pull/3194/files/c96ff85a..499394e9

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

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

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions [v3]

Coleen Phillimore-3
In reply to this post by Harold Seigel-2
On Thu, 25 Mar 2021 21:02:55 GMT, Harold Seigel <[hidden email]> wrote:

>> This is logging so I guess it doesn't really matter.
>
> Changed to passing it as an oop.

that looks good!

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

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions [v3]

Coleen Phillimore-3
In reply to this post by Harold Seigel-2
On Thu, 25 Mar 2021 21:05:45 GMT, Harold Seigel <[hidden email]> wrote:

>> Please review this small change to remove unneeded TRAPS and THREAD parameters from functions in verifier related files.  This change was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and Windows, and tiers 3-5 on Linux x64.
>>
>> Thanks, Harold
>
> Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:
>
>   review comment changes

Marked as reviewed by coleenp (Reviewer).

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

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions [v3]

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
On Thu, 25 Mar 2021 19:14:29 GMT, Coleen Phillimore <[hidden email]> wrote:

>> How about if "ResourceMark rm(THREAD)" is changed to "ResourceMark rm;" ?  Then no Thread* is need in the function.
>
> This seems fine to me.

I think the primary goal should be not avoid calling Thread::current() as much as possible.  Passing Thread* thread around, even as the first parameter just for a ResourceMark seems like unnecessary typing to me.  Getting the current thread isn't expensive.  From experience, if we optimized all the Thread::currents away, we'd get no benefits to performance, and it's nice that it's hidden here.  If we have an impasse here, go ahead and add the Thread parameter at the beginning, but I'd rather not see this extra parameter.

Some previous project that I did (I won't admit how long ago that was), was to go through all the sources in search of Thread::current() and use whatever current thread was passed in, usually via TRAPS. I didn't gain any performance from this but the code looked nicer in the end.

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

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions [v3]

Ioi Lam
On 3/25/21 2:41 PM, Coleen Phillimore wrote:
> On Thu, 25 Mar 2021 19:14:29 GMT, Coleen Phillimore <[hidden email]> wrote:
>
>>> How about if "ResourceMark rm(THREAD)" is changed to "ResourceMark rm;" ?  Then no Thread* is need in the function.
>> This seems fine to me.
> I think the primary goal should be not avoid calling Thread::current() as much as possible.  Passing Thread* thread around, even as the first parameter just for a ResourceMark seems like unnecessary typing to me.  Getting the current thread isn't expensive.  From experience, if we optimized all the Thread::currents away, we'd get no benefits to performance, and it's nice that it's hidden here.  If we have an impasse here, go ahead and add the Thread parameter at the beginning, but I'd rather not see this extra parameter.
>
> Some previous project that I did (I won't admit how long ago that was), was to go through all the sources in search of Thread::current() and use whatever current thread was passed in, usually via TRAPS. I didn't gain any performance from this but the code looked nicer in the end.
>

OK, I agree that whether passing the Thread probably won't make much
difference. The current version looks good to me.

Thanks
- Ioi

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions [v3]

Ioi Lam-2
In reply to this post by Harold Seigel-2
On Thu, 25 Mar 2021 21:05:45 GMT, Harold Seigel <[hidden email]> wrote:

>> Please review this small change to remove unneeded TRAPS and THREAD parameters from functions in verifier related files.  This change was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and Windows, and tiers 3-5 on Linux x64.
>>
>> Thanks, Harold
>
> Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:
>
>   review comment changes

Marked as reviewed by iklam (Reviewer).

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

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions [v3]

David Holmes-2
In reply to this post by Harold Seigel-2
On Thu, 25 Mar 2021 21:05:45 GMT, Harold Seigel <[hidden email]> wrote:

>> Please review this small change to remove unneeded TRAPS and THREAD parameters from functions in verifier related files.  This change was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and Windows, and tiers 3-5 on Linux x64.
>>
>> Thanks, Harold
>
> Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:
>
>   review comment changes

Hi Harold,

Some good cleanup here, but I have a couple of things that need adjusting - see comments inline.

Thanks,
David

src/hotspot/share/classfile/verifier.cpp line 138:

> 136: // Prints the end-verification message to the appropriate output.
> 137: void Verifier::log_end_verification(outputStream* st, const char* klassName, Symbol* exception_name, oop pending_exception) {
> 138:    Thread* THREAD = Thread::current();

This is not needed.

src/hotspot/share/classfile/verifier.cpp line 142:

> 140:   if (pending_exception != NULL) {
> 141:     st->print("Verification for %s has", klassName);
> 142:     oop message = java_lang_Throwable::message(PENDING_EXCEPTION);

This should be pending_exception

src/hotspot/share/classfile/verifier.cpp line 198:

> 196:   log_info(class, init)("Start class verification for: %s", klass->external_name());
> 197:   if (klass->major_version() >= STACKMAP_ATTRIBUTE_MAJOR_VERSION) {
> 198:     ClassVerifier split_verifier(THREAD->as_Java_thread(), klass);

You already have the "jt" JavaThread for this.

src/hotspot/share/classfile/verifier.cpp line 199:

> 197:   if (klass->major_version() >= STACKMAP_ATTRIBUTE_MAJOR_VERSION) {
> 198:     ClassVerifier split_verifier(THREAD->as_Java_thread(), klass);
> 199:     split_verifier.verify_class(THREAD);

Can you add the following comment before verify_class please

// We don't use CHECK here, or on inference_verify below, so that we can log any exception.

Thanks

src/hotspot/share/classfile/verifier.cpp line 695:

> 693:   ConstantPool* cp = _klass->constants();
> 694:   Symbol* const method_sig = cp->symbol_at(sig_index);
> 695:   translate_signature(method_sig, sig_verif_types, CHECK_VERIFY(this));

CHECK_VERIFY is not directly related to exceptions. Are there really no verification errors possible from all of these calls where CHECK_VERIFY has been removed?

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

Changes requested by dholmes (Reviewer).

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions [v3]

David Holmes-2
On Fri, 26 Mar 2021 05:19:23 GMT, David Holmes <[hidden email]> wrote:

>> Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   review comment changes
>
> Hi Harold,
>
> Some good cleanup here, but I have a couple of things that need adjusting - see comments inline.
>
> Thanks,
> David

I forgot to comment that I'm also surprised that annotations never cause any errors, but if invalid in any way are just ignored. I would have thought that any structural deviations from what is required by JVMS would have resulted in a verification error.

David

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

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions [v4]

Harold Seigel-2
In reply to this post by Harold Seigel-2
> Please review this small change to remove unneeded TRAPS and THREAD parameters from functions in verifier related files.  This change was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and Windows, and tiers 3-5 on Linux x64.
>
> Thanks, Harold

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

  Address dholmes's comments

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3194/files
  - new: https://git.openjdk.java.net/jdk/pull/3194/files/499394e9..7fc39ac2

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

  Stats: 6 lines in 1 file changed: 2 ins; 2 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3194.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3194/head:pull/3194

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

Re: RFR: 8264142: Remove TRAPS/THREAD parameters for verifier related functions [v3]

Harold Seigel-2
In reply to this post by David Holmes-2
On Fri, 26 Mar 2021 04:51:31 GMT, David Holmes <[hidden email]> wrote:

>> Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   review comment changes
>
> src/hotspot/share/classfile/verifier.cpp line 138:
>
>> 136: // Prints the end-verification message to the appropriate output.
>> 137: void Verifier::log_end_verification(outputStream* st, const char* klassName, Symbol* exception_name, oop pending_exception) {
>> 138:    Thread* THREAD = Thread::current();
>
> This is not needed.

removed. Thanks for pointing that out.

> src/hotspot/share/classfile/verifier.cpp line 142:
>
>> 140:   if (pending_exception != NULL) {
>> 141:     st->print("Verification for %s has", klassName);
>> 142:     oop message = java_lang_Throwable::message(PENDING_EXCEPTION);
>
> This should be pending_exception

fixed

> src/hotspot/share/classfile/verifier.cpp line 198:
>
>> 196:   log_info(class, init)("Start class verification for: %s", klass->external_name());
>> 197:   if (klass->major_version() >= STACKMAP_ATTRIBUTE_MAJOR_VERSION) {
>> 198:     ClassVerifier split_verifier(THREAD->as_Java_thread(), klass);
>
> You already have the "jt" JavaThread for this.

Fixed.

> src/hotspot/share/classfile/verifier.cpp line 199:
>
>> 197:   if (klass->major_version() >= STACKMAP_ATTRIBUTE_MAJOR_VERSION) {
>> 198:     ClassVerifier split_verifier(THREAD->as_Java_thread(), klass);
>> 199:     split_verifier.verify_class(THREAD);
>
> Can you add the following comment before verify_class please
>
> // We don't use CHECK here, or on inference_verify below, so that we can log any exception.
>
> Thanks

done, thanks for suggesting this.

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

PR: https://git.openjdk.java.net/jdk/pull/3194
12