On 7/20/20 1:53 AM, David Holmes wrote:
Hi Kim,

Thanks for looking at this.

Updated webrev at:

http://cr.openjdk.java.net/~dholmes/8249650/webrev.v2/

I like this cleanup very much!


src/hotspot/share/classfile/javaClasses.cpp
    No comments.

src/hotspot/share/classfile/verifier.cpp
    L298:   JavaThread* thread = (JavaThread*)THREAD;
    L307:   ResourceMark rm(THREAD);
        Since we've gone to the trouble of creating the 'thread' variable,
        I would prefer it to be used instead of THREAD where possible.

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
    L1021:   HandleMark hm;
        Can this be 'hm(THREAD)'? (Not your problem, but while you're
        in that file?)

src/hotspot/share/prims/jni.cpp
    No comments.

src/hotspot/share/prims/jvm.cpp
    L140:   ResourceMark rm;
        Can this be 'rm(THREAD)'? (Not your problem, but while you're
        in that file?)

    L611:   Handle stackStream_h(THREAD, JNIHandles::resolve_non_null(stackStream));
    L617:   objArrayHandle frames_array_h(THREAD, fa);
    L626:   return JNIHandles::make_local(THREAD, result);
        Since we've gone to the trouble of creating the 'jt' variable,
        I would prefer it to be used instead of THREAD where possible.

    L767:   vframeStream vfst(thread);
    L788         return (jclass) JNIHandles::make_local(THREAD, m->method_holder()->java_mirror());
        Can we use 'thread' on L788? (preferred)
        Can we use 'THREAD' on L767? (less preferred)

    L949:   ResourceMark rm(THREAD);
    L951:   Handle class_loader (THREAD, JNIHandles::resolve(loader));
    L955:                            THREAD);
    L957:   Handle protection_domain (THREAD, JNIHandles::resolve(pd));
    L968:   return (jclass) JNIHandles::make_local(THREAD, k->java_mirror());
        Since we've gone to the trouble of creating the 'jt' variable,
        I would prefer it to be used instead of THREAD where possible.

    L986:   JavaThread* jt = (JavaThread*) THREAD;
        This 'jt' is unused and can be deleted (Not your problem, but while you're
        in that file?)

    L1154:   while (*p != '\0') {
    L1155:       if (*p == '.') {
    L1156:           *p = '/';
    L1157:       }
    L1158:       p++;
        Nit - the indents are wrong on L1155-58. (Not your problem, but while you're
        in that file?)

    L1389:   ResourceMark rm(THREAD);
    L1446:     return JNIHandles::make_local(THREAD, result);
    L1460:   return JNIHandles::make_local(THREAD, result);
        Can we use 'thread' on L1389? (preferred) And then the line you
        touched could also be 'thread' and we'll be consistent in this
        function...

    L3287:   oop jthread = thread->threadObj();
    L3288:   assert (thread != NULL, "no current thread!");
        I think the assert is wrong. It should be:

            assert(jthread != NULL, "no current thread!");

        If 'thread == NULL', then we would have crashed at L3287.
        Also notice that I deleted the extra ' ' before '('. (Not
        your problem, but while you're in that file?)

    L3289:   return JNIHandles::make_local(THREAD, jthread);
        Can you use 'thread' instead of 'THREAD' here for consistency?

    L3681:     method_handle = Handle(THREAD, JNIHandles::resolve(method));
    L3682:     Handle receiver(THREAD, JNIHandles::resolve(obj));
    L3683:     objArrayHandle args(THREAD, objArrayOop(JNIHandles::resolve(args0)));
    L3685:     jobject res = JNIHandles::make_local(THREAD, result);
        Can you use 'thread' instead of 'THREAD' here for consistency?

    L3705:   objArrayHandle args(THREAD, objArrayOop(JNIHandles::resolve(args0)));
    L3707   jobject res = JNIHandles::make_local(THREAD, result);
        Can you use 'thread' instead of 'THREAD' here for consistency?

src/hotspot/share/prims/methodHandles.cpp
    No comments.

src/hotspot/share/prims/methodHandles.hpp
    No comments.

src/hotspot/share/prims/unsafe.cpp
    No comments.

src/hotspot/share/prims/whitebox.cpp
    No comments.

src/hotspot/share/runtime/jniHandles.cpp
    No comments.

src/hotspot/share/runtime/jniHandles.hpp
    No comments.

src/hotspot/share/services/management.cpp
    No comments.


None of my comments above are "must do". If you choose to make the
changes, a new webrev isn't required, but would be useful for a
sanity check.

Thumbs up.

Dan



On 20/07/2020 3:22 pm, Kim Barrett wrote:
On Jul 20, 2020, at 12:16 AM, David Holmes <david.hol...@oracle.com> wrote:

Subject line got truncated by accident ...

On 20/07/2020 11:06 am, David Holmes wrote:
Bug: https://bugs.openjdk.java.net/browse/JDK-8249650
webrev: http://cr.openjdk.java.net/~dholmes/8249650/webrev/
This is a simple cleanup that touches files across a number of VM areas - hence the cross-post. Whilst working on a different JNI fix I noticed that in most cases in jni.cpp we were using the following form of make_local:
JNIHandles::make_local(env, obj);
and what that form does is first extract the thread from the JNIEnv:
JavaThread* thread = JavaThread::thread_from_jni_environment(env);
return thread->active_handles()->allocate_handle(obj);
but there is also another, faster, variant for when you already have the "thread":
jobject JNIHandles::make_local(Thread* thread, oop obj) {
   return thread->active_handles()->allocate_handle(obj);
}
When you look at the JNI_ENTRY wrapper (and related JVM_ENTRY, WB_ENTRY, UNSAFE_ENTRY etc) it has already extracted the thread from the JNIEnv:
     JavaThread* thread=JavaThread::thread_from_jni_environment(env);
and further defined:
     Thread* THREAD = thread;
so we always already have direct access to the "thread" available (or indirect via TRAPS), and in fact we can end up removing the make_local(JNIEnv* env, oop obj) variant altogether. Along the way I spotted some related issues with unnecessary use of Thread::current() when it is already available from TRAPS, and some other cases where we extracted the JNIEnv from a thread only to later extract the thread from the JNIEnv.
Testing: tiers 1 - 3
Thanks,
David
-----

------------------------------------------------------------------------------
src/hotspot/share/classfile/javaClasses.cpp
  439     JNIEnv *env = thread->jni_environment();

Since env is no longer used on the next line, move this down to where
it is used, at line 444.

Fixed.

------------------------------------------------------------------------------
src/hotspot/share/classfile/verifier.cpp
  299   JNIEnv *env = thread->jni_environment();

env now seems to only be used at line 320.  Move this closer.

Fixed.

------------------------------------------------------------------------------
src/hotspot/share/prims/jni.cpp
  743     result = JNIHandles::make_local(THREAD, result_handle());

jni_PopLocalFrame is now using a mix of "thread" and "THREAD", where
previously it just used "thread". Maybe this change shouldn't be made?
Or can the other uses be changed to THREAD for consistency?

"thread" and "THREAD" are interchangeable for anything expecting a "Thread*" (and somewhat surprisingly a number of API's that only work for JavaThreads actually take a Thread*. :( ). I had choice between trying to be file-wide consistent with the make_local calls, versus local-code consistent, and used THREAD as it is available in both JNI_ENTRY and via TRAPS. But I can certainly make a local change to "thread" for local consistency.

------------------------------------------------------------------------------
src/hotspot/share/prims/jvm.cpp

The calls to JvmtiExport::post_vm_object_alloc have to use "thread"
instead of "THREAD", even though other places nearby are using
"THREAD".  That inconsistency is kind of unfortunate, but doesn't seem
easily avoidable.

Everything that uses THREAD in a JVM_ENTRY method can be changed to use "thread" instead. But I'm not sure it's a consistency worth pursuing at least as part of these changes (there are likely similar issues with most of the touched files).

Thanks,
David

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


Reply via email to