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

8264142: Remove TRAPS/THREAD parameters for verifier related functions #3194

Closed
wants to merge 4 commits into from

Conversation

@hseigel
Copy link
Member

@hseigel hseigel commented Mar 25, 2021

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


Progress

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

Issue

  • JDK-8264142: Remove TRAPS/THREAD parameters for verifier related functions

Reviewers

Download

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

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 25, 2021

👋 Welcome back hseigel! 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 openjdk bot added the rfr label Mar 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 25, 2021

@hseigel The following label will be automatically applied to this pull request:

  • hotspot-runtime

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.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 25, 2021

Copy link

@gerard-ziemski gerard-ziemski left a comment

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?

src/hotspot/share/classfile/classFileParser.cpp Outdated Show resolved Hide resolved
src/hotspot/share/classfile/verifier.cpp Outdated Show resolved Hide resolved
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 25, 2021

Mailing list message from Ioi Lam on hotspot-runtime-dev:

On 3/25/21 11:02 AM, Harold Seigel wrote:

On Thu, 25 Mar 2021 17:54:53 GMT, Coleen Phillimore <coleenp at openjdk.org> 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

src/hotspot/share/classfile/verifier.cpp Outdated Show resolved Hide resolved
@openjdk
Copy link

@openjdk openjdk bot commented Mar 25, 2021

@hseigel This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8264142: Remove TRAPS/THREAD parameters for verifier related functions

Reviewed-by: gziemski, coleenp, iklam, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 32 new commits pushed to the master branch:

  • 6678b01: 8264328: Broken license in javax/swing/JComboBox/8072767/bug8072767.java
  • 2fa6a3c: 8264006: Fix AOT library loading on CPUs with 256-byte dcache line
  • c986457: 8264329: Z cannot be 1 for Diffie-Hellman key agreement
  • a209ed0: 8263670: pmap and pstack in jhsdb do not work on debug server
  • 38e0a58: 8264273: macOS: zero VM is broken due to no member named 'is_cpu_emulated' after JDK-8261966
  • c9d2d02: 8263632: Improve exception handling of APIs in classLoader.cpp
  • 59ed1fa: 8264087: Use the blessed modifier order in jdk.jconsole
  • 054e0a4: 8264017: Correctly report inlined frame in JFR sampling
  • d6bb153: 8264240: [macos_aarch64] enable appcds support after JDK-8263002
  • 7284f01: 8262110: DST starts from incorrect time in 2038
  • ... and 22 more: https://git.openjdk.java.net/jdk/compare/f69afba52735008613f0ede7d650372e95e9a6e0...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

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

@openjdk openjdk bot added the ready label Mar 25, 2021
@iklam
iklam approved these changes Mar 26, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 26, 2021

Mailing list message from Ioi Lam on hotspot-runtime-dev:

On 3/25/21 2:41 PM, Coleen Phillimore wrote:

On Thu, 25 Mar 2021 19:14:29 GMT, Coleen Phillimore <coleenp at openjdk.org> 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

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Harold,

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

Thanks,
David

void Verifier::log_end_verification(outputStream* st, const char* klassName, Symbol* exception_name, oop pending_exception) {
Thread* THREAD = Thread::current();

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

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 26, 2021
Member

This should be pending_exception

This comment has been minimized.

@hseigel

hseigel Mar 26, 2021
Author Member

fixed

void Verifier::log_end_verification(outputStream* st, const char* klassName, Symbol* exception_name, TRAPS) {
if (HAS_PENDING_EXCEPTION) {
void Verifier::log_end_verification(outputStream* st, const char* klassName, Symbol* exception_name, oop pending_exception) {
Thread* THREAD = Thread::current();

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 26, 2021
Member

This is not needed.

This comment has been minimized.

@hseigel

hseigel Mar 26, 2021
Author Member

removed. Thanks for pointing that out.

@@ -193,7 +195,7 @@ bool Verifier::verify(InstanceKlass* klass, bool should_verify_class, TRAPS) {

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

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 26, 2021
Member

You already have the "jt" JavaThread for this.

This comment has been minimized.

@hseigel

hseigel Mar 26, 2021
Author Member

Fixed.

@@ -193,7 +195,7 @@ bool Verifier::verify(InstanceKlass* klass, bool should_verify_class, TRAPS) {

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

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 26, 2021
Member

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

This comment has been minimized.

@hseigel

hseigel Mar 26, 2021
Author Member

done, thanks for suggesting this.

// Translate the signature into verification types.
ConstantPool* cp = _klass->constants();
Symbol* const method_sig = cp->symbol_at(sig_index);
translate_signature(method_sig, sig_verif_types, CHECK_VERIFY(this));

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 26, 2021
Member

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?

This comment has been minimized.

@hseigel

hseigel Mar 26, 2021
Author Member

The calls where CHECK_VERIFY's were removed are to functions that deal with signatures. These methods do not check for errors because the signatures have already been verified by classFileParser.cpp in functions such as verify_legal_method_signature() and verify_legal_field_signature().

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 29, 2021
Member

Okay - thanks for confirming that.

@dholmes-ora
Copy link
Member

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

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

Copy link
Member

@dholmes-ora dholmes-ora left a comment

LGTM!

Thanks,
David

// Translate the signature into verification types.
ConstantPool* cp = _klass->constants();
Symbol* const method_sig = cp->symbol_at(sig_index);
translate_signature(method_sig, sig_verif_types, CHECK_VERIFY(this));

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 29, 2021
Member

Okay - thanks for confirming that.

@hseigel
Copy link
Member Author

@hseigel hseigel commented Mar 29, 2021

Thanks Gerard, Ioi, Coleen, and David for the reviews!

/integrate

@openjdk openjdk bot closed this Mar 29, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 29, 2021

@hseigel Since your change was applied there have been 38 commits pushed to the master branch:

  • aefc156: 8264179: [TESTBUG] Some compiler tests fail when running without C2
  • 30b4b17: 8264334: Use the blessed modifier order in jdk.jpackage
  • 8ee9a05: 8259623: JfrTypeSet::_subsystem_callback is left dangling after use
  • 1f54ed0: 8264096: slowdebug jvm crashes when StrInflatedCopy match rule is not supported
  • 447e0df: 8263769: simplify PhaseMacroExpand::extract_call_projections()
  • 99b4bab: 8263188: JSSE should fail fast if there isn't supported signature algorithm
  • 6678b01: 8264328: Broken license in javax/swing/JComboBox/8072767/bug8072767.java
  • 2fa6a3c: 8264006: Fix AOT library loading on CPUs with 256-byte dcache line
  • c986457: 8264329: Z cannot be 1 for Diffie-Hellman key agreement
  • a209ed0: 8263670: pmap and pstack in jhsdb do not work on debug server
  • ... and 28 more: https://git.openjdk.java.net/jdk/compare/f69afba52735008613f0ede7d650372e95e9a6e0...master

Your commit was automatically rebased without conflicts.

Pushed as commit 19a6ac4.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants