On Wed, 3 Mar 2021 23:08:06 GMT, David Holmes <[email protected]> wrote:
>> ObjectMonitors can only be used by JavaThreads (modulo some interactions
>> with hashcodes and deflation) but we use "Thread*" almost everywhere mainly
>> due to use of TRAPS (and TRAPS will itself use JavaThread once JDK-8252685
>> is done). Also some uses of TRAPS in the API's are wrong as, for example,
>> monitor entry can never throw an exception.
>>
>> So this cleanup tackles:
>> - remove incorrect use of TRAPS
>> - change "Thread*" to "JavaThread*" where applicable
>> - don't use THREAD for things not related to exception processing
>> - standardise the naming so that we have "JavaThread* current" rather a mix
>> if Self/THREAD/jt etc.
>> - remove unnecessary as_Java_thread() conversions
>> - other misc cleanup I noticed in some functions
>>
>> The cleanup is predominantly in objectMonitor.* and synchronizer.* but with
>> a fan out to the users of those APIs. No attempt is made to cleanup the
>> callers beyond ensuring we have a suitable JavaThread reference for the
>> calls.
>>
>> Thanks,
>> David
>
> David Holmes has updated the pull request incrementally with one additional
> commit since the last revision:
>
> More pointer declaration style fixups
I think I only have nits and typos.
Thanks for doing this massive cleanup!
src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 261:
> 259: volatile_nonstatic_field(ObjectMonitor, _cxq,
> ObjectWaiter*) \
> 260: volatile_nonstatic_field(ObjectMonitor, _EntryList,
> ObjectWaiter*) \
> 261: volatile_nonstatic_field(ObjectMonitor, _succ,
> JavaThread*) \
nit - please fix the indent before the backslash...
src/hotspot/share/oops/instanceKlass.cpp line 946:
> 944: HandleMark hm(THREAD);
> 945: Handle h_init_lock(THREAD, init_lock());
> 946: ObjectLocker ol(h_init_lock, jt);
I was going to mumble about adding a new 'jt' here, but this isn't an
ObjectMonitor related file so you probably held off here.
src/hotspot/share/runtime/objectMonitor.hpp line 49:
> 47: ObjectWaiter* volatile _next;
> 48: ObjectWaiter* volatile _prev;
> 49: JavaThread* _thread;
So no more uses of ObjectWaiter by non-JavaThreads?
src/hotspot/share/runtime/synchronizer.cpp line 437:
> 435: BiasedLocking::revoke(obj, current);
> 436: } else {
> 437: guarantee(false, "VMThread should not be involved with
> ObjectMonitor");
Interesting change. Seems out of context for this change.
Since you have "guarantee(false," you can use "fatal(" instead...
src/hotspot/share/runtime/synchronizer.cpp line 612:
> 610: ObjectMonitor* monitor = inflate(current, obj, inflate_cause_jni_exit);
> 611: // If this thread has locked the object, exit the monitor. We
> 612: // intentionally do not use CHECK on check_owner because we must exit
> the
s/CHECK on check_owner/check_owner/
src/hotspot/share/runtime/synchronizer.cpp line 678:
> 676: // after ownership is regained.
> 677: ObjectMonitor* monitor = inflate(current, obj(), inflate_cause_wait);
> 678: monitor->wait(0 /* wait-forever */, false /* not interruptible */,
> current);
Didn't expect to see this change from "millis" to "0" either.
Seems out of context for this change.
Update: I see that you deleted the 'millis' param now. I missed that before.
src/hotspot/share/runtime/synchronizer.cpp line 1071:
> 1069: // monitors_iterate() is only called at a safepoint or when the
> 1070: // target thread is suspended or when the target thread is
> 1071: // operating on itcurrent. The current closures in use today are
typo - s/itcurrent/itself/
src/hotspot/share/runtime/synchronizer.hpp line 206:
> 204: void wait(TRAPS) { ObjectSynchronizer::wait(_obj, 0, CHECK); } //
> wait forever
> 205: void notify_all(TRAPS) { ObjectSynchronizer::notifyall(_obj, CHECK); }
> 206: void wait_uninterruptibly(JavaThread* current) {
> ObjectSynchronizer::wait_uninterruptibly(_obj, current); }
Any reason for not use 'current_thread' here?
src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
line 516:
> 514: public final int objectMonitorCxq =
> getFieldOffset("ObjectMonitor::_cxq", Integer.class, "ObjectWaiter*", -1,
> jdk13Backport);
> 515: public final int objectMonitorEntryList =
> getFieldOffset("ObjectMonitor::_EntryList", Integer.class, "ObjectWaiter*",
> -1, jdk13Backport);
> 516: public final int objectMonitorSucc =
> getFieldOffset("ObjectMonitor::_succ", Integer.class, JDK < 17 ? "Thread*" :
> "JavaThread*", -1, jdk13Backport);
That makes my brain hurt...
-------------
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2802
