Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
Daniil,
I forgot to ask you an output log of new tests.
Could you, please, inline it in your reply?
Thanks,
Serguei
On 7/20/20 18:48, serguei.spit...@oracle.com wrote:
Hi Daniil,
The fix looks pretty good to me.
Just minor comments.
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html
2519 int skipped = 0; // skip default methods from superinterface (see JDK-8216324)
You can say just: // skip overpass methods
There is probably no need to list the bug number.
2523 // Depending on can_maintain_original_method_order capability
2524 // use the original method ordering indices stored in the class, so we can emit
2525 // jmethodIDs in the order they appeared in the class file
2526 // or just copy in any order
Could you, please re-balance the comment a little bit?
Also, the comment has to be terminated with a dot.
Replace: "or just copy in any order" =>
"or just copy in current order".
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassMethods/getclmthd007.java.frames.html
114 default void default_method() { } // should never be seen
The comment above is not clear. Should never be seen in what
context?
117 interface OuterInterface1 extends DefaultInterface {
An extra space before "extends".
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/serviceability/jvmti/GetClassMethods/InterfaceDefMethods.java.html
I like the test simplicity.
Default methods are always in interfaces.
I'd suggest to rename the test to something like:
DefaultMethods.java or OverpassMethods.java.
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/serviceability/jvmti/GetClassMethods/libInterfaceDefMethods.cpp.html
44 if (options != NULL && strcmp(options, "maintain_original_method_order") == 0) {
45 printf("maintain_original_method_order: true\n");
...
54 } else {
55 printf("maintain_original_method_order: false\n");
I'd suggest to remove the lines 54 and 55
and adjust the line 45:
printf("Enabled capability: maintain_original_method_order:
true\n");
88 jobject m = env->ToReflectedMethod(klass, methods[i], (modifiers & 8) == 8);
It is better to replace 8 with a symbolic
constant.
Thanks,
Serguei
On 7/20/20 16:57, Alex Menkov wrote:
Looks
good to me :)
Thanks for handling this.
--alex
On 07/20/2020 12:00, Daniil Titov wrote:
Please review change [1] that fixes
GetClassMethods behavior in cases if a default method is
present in a super interface. Currently for such cases the
information GetClassMethods returns for the sub-interface or
implementing class incorrectly includes inherited not
default methods.
The fix ( thanks to Alex for providing this patch) ensures
that overpass methods are filtered out in the returns. The
fix also applies a change in the existing test as David
suggested in the comments to the issue [2].
Testing: Mach5 tier1-tier3 tests successfully passed.
[1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/
[2] https://bugs.openjdk.java.net/browse/JDK-8216324
Thank you,
Daniil
Re: RFR (M) 8249768: Move static oops and NullPointerException oops from Universe into OopStorage
Hi Coleen, cc'ing serviceability due to SA changes. On 21/07/2020 6:53 am, coleen.phillim...@oracle.com wrote: Summary: Move static oops into OopStorage and make NPE oops an objArrayOop. I've broken up moving oops in Universe to OopStorage into several parts. This change moves the global static oops. These OopHandles are not released. Overall looks good. But two things ... 1. naming ! // preallocated error objects (no backtrace) ! static OopHandle_out_of_memory_error; // array of preallocated error objects with backtrace ! static OopHandle _preallocated_out_of_memory_error_array; Both of these are pre-allocated arrays of OopHandles, differing only in whether the underlying OOME has a backtrace or not. I find the newly introduced _out_of_memory_error unclear in that regard. At a minimum could _out_of_memory_error become _out_of_memory_errors ? But ideally can we name these two arrays in a more distinguishable way? 2. SA You've simply deleted all the SA/vmstructs code that referenced the oops that are no longer present. Does the SA not care about these things and need access to them? (I don't know hence cc to serviceability folk.) Thanks, David - This has been tested with tier1-3 and on also remote-build -b linux-arm32,linux-ppc64le-debug,linux-s390x-debug,linux-x64-zero. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8249768.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8249768 Thanks, Coleen
Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
Hi Daniil,
The fix looks pretty good to me.
Just minor comments.
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html
2519 int skipped = 0; // skip default methods from superinterface (see JDK-8216324)
You can say just: // skip overpass methods
There is probably no need to list the bug number.
2523 // Depending on can_maintain_original_method_order capability
2524 // use the original method ordering indices stored in the class, so we can emit
2525 // jmethodIDs in the order they appeared in the class file
2526 // or just copy in any order
Could you, please re-balance the comment a little bit?
Also, the comment has to be terminated with a dot.
Replace: "or just copy in any order" => "or just copy in current order".
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassMethods/getclmthd007.java.frames.html
114 default void default_method() { } // should never be seen
The comment above is not clear. Should never be seen in what
context?
117 interface OuterInterface1 extends DefaultInterface {
An extra space before "extends".
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/serviceability/jvmti/GetClassMethods/InterfaceDefMethods.java.html
I like the test simplicity.
Default methods are always in interfaces.
I'd suggest to rename the test to something like:
DefaultMethods.java or OverpassMethods.java.
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/serviceability/jvmti/GetClassMethods/libInterfaceDefMethods.cpp.html
44 if (options != NULL && strcmp(options, "maintain_original_method_order") == 0) {
45 printf("maintain_original_method_order: true\n");
...
54 } else {
55 printf("maintain_original_method_order: false\n");
I'd suggest to remove the lines 54 and 55
and adjust the line 45:
printf("Enabled capability: maintain_original_method_order:
true\n");
88 jobject m = env->ToReflectedMethod(klass, methods[i], (modifiers & 8) == 8);
It is better to replace 8 with a symbolic
constant.
Thanks,
Serguei
On 7/20/20 16:57, Alex Menkov wrote:
Looks
good to me :)
Thanks for handling this.
--alex
On 07/20/2020 12:00, Daniil Titov wrote:
Please review change [1] that fixes
GetClassMethods behavior in cases if a default method is present
in a super interface. Currently for such cases the information
GetClassMethods returns for the sub-interface or implementing
class incorrectly includes inherited not default methods.
The fix ( thanks to Alex for providing this patch) ensures that
overpass methods are filtered out in the returns. The fix also
applies a change in the existing test as David suggested in the
comments to the issue [2].
Testing: Mach5 tier1-tier3 tests successfully passed.
[1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/
[2] https://bugs.openjdk.java.net/browse/JDK-8216324
Thank you,
Daniil
Re: [15?] RFR (S): 8249192: MonitorInfo stores raw oops across safepoints
Hi Thomas, On 21/07/2020 12:49 am, Thomas Schatzl wrote: Forwarding to hotspot-dev where it belongs after wrongly sending to hotspot-gc-dev. This touches serviceability code as well so cc'ing for good measure. Thanks for taking this one on as it wasn't actually a GC issue! Forwarded Message Subject: [15?] RFR (S): 8249192: MonitorInfo stores raw oops across safepoints Date: Mon, 20 Jul 2020 12:07:38 +0200 From: Thomas Schatzl To: hotspot-...@openjdk.java.net Hi all, can I get some reviews to handle'ize some raw oops in the MonitorInfo class? (Afaiu only) in LiveFrameStream::monitors_to_object_array() we try to allocate an objArray with raw oops held in the MonitorInfo class that are passed in a GrowableArray. This allocation can lead to a garbage collection, with the usual random crashes. Right - seems so obvious now. Took me a while to convince myself no such similar problem was lurking in the JVM TI code. This change changes the raw oops in MonitorInfo to Handles, My main concern here was whether the MonitorInfo objects are thread confined. For the StackWalker API we are always dealing with the current thread so that is fine. For JVM TI, in mainline, we may be executing code in the calling thread or the target thread; and in older releases it will be the VMThread at a safepoint. But it seems that the MonitorInfo's are confined to whichever thread that is, and so Handle usage is safe. and adds a few HandleMarks along the way to make these handles go away asap. That, and the ResourceMark changes, were a bit hard to follow. Basically a HandleMark is now present in the scope of, or just above, the call to monitors(). The need for the additional ResourceMarks is far from clear though. In particular I wonder if the RM introduced in Deoptimization::revoke_from_deopt_handler interacts with the special DeoptResourceMark in its caller Deoptimization::fetch_unroll_info_helper? (I have no idea what a DeoptResourceMark is.) This issue has been introduced in JDK-8140450: Implement Stack-Walking API in jdk9. The CR has been triaged as P3, but I would like to ask whether it might be good to increase its priority to P2 and apply for inclusion in 15. My arguments are as follows: - the original issue why I started looking at this were lots of seemingly random crashes (5 or 6 were reported and the change temporarily backed out for this reason) in tier8 with a g1 change that changed young gen sizing. These crashes including that young gen sizing change are all gone now with this bugfix. I.e. this suggests that so far we seem to have not encountered this issue more frequently due to pure luck wrt to generation sizing. - it affects all collectors (naturally). - there are quite a few user reported random crashes with IntelliJ and variants, which due to the nature of IDEs tending to retrieve stack traces fairly frequently would be more affected than usual. So I suspect at least some of them to be caused by this issue, these are the only raw oops I am aware of. My understanding of the cause and fix is fairly good, but I am no expert in this area, so I would like to defer to you about this suggestion. The change is imo important enough to be backported to 11 and 15 anyway, but the question is about the risk/reward tradeoff wrt to bringing it to 15 and not 15.0.1. I'd classify this as a P2 without doubt. As Dan noted there is no workaround as such. CR: https://bugs.openjdk.java.net/browse/JDK-8249192 Webrev: http://cr.openjdk.java.net/~tschatzl/8249192/webrev/ src/hotspot/share/runtime/deoptimization.cpp The code in collect_monitors takes the monitor owner oop and Handelises it to add to its own GrowableArray of Handles. Is it worth exposing the MonitorInfo owner() in Handle form to avoid this unwrapping and re-wrapping? src/hotspot/share/runtime/vframe.hpp I agree with Coleen that the MonitorInfo constructor should not take a Thread* but should itself materialize and use Thread::current(). Thanks, David - Testing: tier1-5,tier8 (with some unrelated changes), 1800+ runs of the reproducer Thanks, Thomas
Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
Looks good to me :) Thanks for handling this. --alex On 07/20/2020 12:00, Daniil Titov wrote: Please review change [1] that fixes GetClassMethods behavior in cases if a default method is present in a super interface. Currently for such cases the information GetClassMethods returns for the sub-interface or implementing class incorrectly includes inherited not default methods. The fix ( thanks to Alex for providing this patch) ensures that overpass methods are filtered out in the returns. The fix also applies a change in the existing test as David suggested in the comments to the issue [2]. Testing: Mach5 tier1-tier3 tests successfully passed. [1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8216324 Thank you, Daniil
RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
Please review change [1] that fixes GetClassMethods behavior in cases if a default method is present in a super interface. Currently for such cases the information GetClassMethods returns for the sub-interface or implementing class incorrectly includes inherited not default methods. The fix ( thanks to Alex for providing this patch) ensures that overpass methods are filtered out in the returns. The fix also applies a change in the existing test as David suggested in the comments to the issue [2]. Testing: Mach5 tier1-tier3 tests successfully passed. [1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8216324 Thank you, Daniil
Re: RFR (M) 8249650: Optimize JNIHandle::make_local thread variable usage
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
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 t
Re: RFR 8247878: Move Management strong oops to OopStorage
David, thank you for the review and help.
Coleen
On 7/20/20 12:47 AM, David Holmes wrote:
Hi Coleen,
On 18/07/2020 7:29 am, coleen.phillim...@oracle.com wrote:
I've corrected this change with Kim's and David's feedback, and
retested with tier1-3. This is much better.
Yes this is better. :) Thanks to Kim for clarifying the
acquire/release issue.
LGTM.
Thanks,
David
-
incremental webrev at
http://cr.openjdk.java.net/~coleenp/2020/8247878.02.incr/webrev
full webrev at
http://cr.openjdk.java.net/~coleenp/2020/8247878.02/webrev
Thanks,
Coleen
On 7/17/20 10:50 AM, coleen.phillim...@oracle.com wrote:
Hi Kim, Thank you for reviewing this.
On 7/17/20 5:02 AM, Kim Barrett wrote:
On Jul 16, 2020, at 11:01 AM, coleen.phillim...@oracle.com wrote:
Summary: Use OopStorage for strong oops stored with memory and
thread sampling and dumping, and remove oops_do and GC calls.
These use OopStorageSet::vm_global() OopStorage for now. I'll
change the thread sampling oops to use a new OopStorage once the
GC code is changed to nicely allow additional oop storages. The
memory pool oops are never deleted once created, so they should
stay in vm_global() oop storage.
Tested with tiers 1-3 (tiers 4-6 with other changes) and
javax/management tests. I timed the tests to see if there was any
noticeable performance difference, and there was not.
open webrev at
http://cr.openjdk.java.net/~coleenp/2020/8247878.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8247878
Thanks,
Coleen
--
src/hotspot/share/oops/oopHandle.inline.hpp
50 if (peek() != NULL) {
Adding that seems like an unrelated change, and it's not clear to me
why this is being done.
This was to save null checks in all the callers, particularly here:
ThreadSnapshot::~ThreadSnapshot() {
+ _blocker_object.release(OopStorageSet::vm_global());
+ _blocker_object_owner.release(OopStorageSet::vm_global());
+ _threadObj.release(OopStorageSet::vm_global());
+
--
src/hotspot/share/services/lowMemoryDetector.cpp
299 if (_sensor_obj.peek() != NULL) {
300 InstanceKlass* sensorKlass =
Management::sun_management_Sensor_klass(CHECK);
301 Handle sensor_h(THREAD, _sensor_obj.resolve());
I see no reason for a peek operation here, and think it just makes the
code harder to understand. Just unconditionally resolve the sensor.
Similarly here:
364 if (_sensor_obj.peek() != NULL) {
365 InstanceKlass* sensorKlass =
Management::sun_management_Sensor_klass(CHECK);
366 Handle sensor(THREAD, _sensor_obj.resolve());
I can move the NULL check down after the Handle. I was mostly
keeping the existing pattern.
--
src/hotspot/share/services/memoryManager.cpp
136 _memory_mgr_obj =
AtomicOopHandle(OopStorageSet::vm_global(), mgr_obj);
There is a race here. The handle constructor allocates the oopstorage
entry and then does a release_store of the value into it. After (in
source order) the handle is constructed, it is copied into
_memory_mgr_obj, which is just a copy-assign of the oop* from
oopstorage. There's nothing to prevent that copy-assign from being
reordered before the store of the value into the oopstorage, either by
the compiler or the hardware.
Ok, I see that.
The _obj in _memory_mgr_obj is being read without locking and may be
written by another thread, so should itself be appropriate atomic.
It's not clear what the semantics of this new kind of handle are
supposed to be, but I think as used for _memory_mgr_obj there are
problems.
The same is true for _memory_pool_obj.
I think the atomicity here is in the wrong place. The pointee of the
oop* doesn't need atomic access, the oop* itself does. At least I
think so; both _memory_mgr_obj and _memory_pool_obj are lazily
assigned on first use and never change after that, right?
Yes. volatile is in the wrong place.
One way to do that would be that the type of _memory_mgr_obj is `oop*
volatile`, and is initialized as
oop* oop_ptr = ... allocate oopstorage entry ...
NativeAccess<>::oop_store(oop_ptr, value);
Atomic::release_store(&_memory_mgr_obj, oop_ptr);
Alternatively, use
volatile OopHandle _memory_mgr_obj;
Atomic::release_store(&_memory_mgr_obj, OopHandle(...));
and teach Atomic how to deal with OopHandle by defining a
PrimitiveConversions::Translator for it.
The declaration would be
volatile OopHandle _memory_mgr_obj;
and accesses would be
Atomic::load_acquire(&_memory_mgr_obj).resolve();
And AtomicOopHandle isn't useful and should be discarded.
Ok, yes, this is exactly what I want. And David will be happy
because he didn't like AtomicOopHandle.
Thanks for catching this and your help.
Coleen
--
Re: RFR 8247878: Move Management strong oops to OopStorage
Kim, Thank you for your help. Coleen On 7/20/20 1:25 AM, Kim Barrett wrote: On Jul 20, 2020, at 12:47 AM, David Holmes wrote: Hi Coleen, On 18/07/2020 7:29 am, coleen.phillim...@oracle.com wrote: I've corrected this change with Kim's and David's feedback, and retested with tier1-3. This is much better. Yes this is better. :) Thanks to Kim for clarifying the acquire/release issue. LGTM. Thanks, David Looks good.
RFR(S) 8249293: Unsafe stackwalk in VM_GetOrSetLocal::doit_prologue()
Hi, please help review this fix for VM_GetOrSetLocal. It moves the unsafe stackwalk from the vm operation prologue before the safepoint into the doit() method executed at the safepoint. Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.0/index.html Bug:https://bugs.openjdk.java.net/browse/JDK-8249293 According to the JVMTI spec on local variable access it is not required to suspend the target thread T [1]. The operation will simply fail with JVMTI_ERROR_NO_MORE_FRAMES if T is executing bytecodes. It will succeed though if T is blocked because of synchronization or executing some native code. The issue is that in the latter case the stack walk in VM_GetOrSetLocal::doit_prologue() to prepare the access to the local variable is unsafe, because it is done before the safepoint and it races with T returning to execute bytecodes making its stack not walkable. The included test shows that this can crash the VM if T wins the race. Manual testing: - new test test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java - test/hotspot/jtreg/vmTestbase/nsk/jvmti - test/hotspot/jtreg/serviceability/jvmti Nightly regression tests @SAP: JCK and JTREG, also in Xcomp mode, SPECjvm2008, SPECjbb2015, Renaissance Suite, SAP specific tests with fastdebug and release builds on all platforms Thanks, Richard. [1] https://docs.oracle.com/en/java/javase/14/docs/specs/jvmti.html#local
Re: RFR (M) 8249650: Optimize JNIHandle::make_local thread variable usage
Thanks Kim! David On 20/07/2020 4:15 pm, Kim Barrett wrote: On Jul 20, 2020, at 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/ Looks good. On 20/07/2020 3:22 pm, Kim Barrett wrote: On Jul 20, 2020, at 12:16 AM, David Holmes wrote: 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. I don’t feel strongly either way. It just struck me as a little odd to have the mix in close proximity, especially since I think consistently using either one might work in this function. But being consistent about make_local usage has something to be said for it too. 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). Yeah, it’s not really obvious whether to use THREAD or thread in some cases. But I agree that addressing any inconsistencies there is mostly out of scope for this change.
