Hi again Dan,
(I've given up trying to figure out how PR review emails get split up. :) )
Trimming ...
On 5/03/2021 9:46 am, Daniel D.Daugherty wrote:
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...
Fixed.
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.
Yes, and "jt" already exists as the JavaThread manifestation of Thread.
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?
See other email. AFAIA non-JavaThreads can wait on ObjectMonitors.
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...
This is an incomplete change. Earlier I still had TRAPS on enter() even
though it must be a JavaThread so I changed the BL code fragment to the
above. But then when I realized enter() never produces an exception, the
TRAPS became JavaThread* and so it is impossible to be at a safepoint
(or in the VMThread.). So I've reformulated that block. Thanks for noticing!
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/
No, the comment is explaining why we do not have this:
bool owned = monitor->check_owner(CHECK);
if (owned) {
monitor->exit(true, current);
}
the exception we are concerned about is not the one that might be posted
by check_owner, but any pre-existing pending exception that might be
present. We must release the monitor regardless.
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.
Yes a "target of opportunity" given the other changes to this method.
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/
Well spotted! (Did that on purpose just to see who was paying attention
- NOT! :) )
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?
?? I'm using "current" everywhere.
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...
Yeah it needs to maintain backward compatibility.
Thanks for the review!
David
-----
-------------
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2802
