On 10/1/19 6:21 PM, David Holmes wrote:
Hi Coleen,
Thanks for taking a look.
On 2/10/2019 7:04 am, [email protected] wrote:
http://cr.openjdk.java.net/~dholmes/8231289/webrev/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html
I think it's odd that PROPER_TRANSITIONS looked like the right thing
to do, but we always took the alternate path with the large comment
about why it is evil. Can we have an RFE to move this special
transition to interfaceSupport.inline.hpp as a supported transition,
so it is with the other ones? So we don't miss it if we changed more
things in the transitions.
I suppose we could - but I honestly don't think it is worth the
effort. Once this is disentangled from ObjectMonitor we can blissfully
ignore it and should never need to change it. The transition from one
safepoint-safe state to another is safe as the comment notes, and not
really "evil" IMO. IIRC we make the same observation elsewhere about
moving between safe states.
I wonder how many other places in the VM we've sprinkled around the code
to change thread states.
Should this transition check for NoSafepointVerifier, for example?
I have no idea what the NSV would mean in this context. JavaThreads
using raw monitors have no interaction with safepoints whilst using
them. The thread is always in a safepoint-safe state and so ignored by
the safepoint logic. We don't care how many safepoints might occur
whilst executing this code.
You're right, we're changing from native to blocked so yes, it doesn't
need to check that safepoints are ok.
http://cr.openjdk.java.net/~dholmes/8231289/webrev/src/hotspot/share/runtime/objectMonitor.hpp.frames.html
39 // ParkEvent instead. Beware, however, that the JVMTI code
40 // knows about ObjectWaiters, so we'll have to reconcile that
code.
41 // See next_waiter(), first_waiter(), etc.
Comment should be removed.
No that is about JVMTI, not JVMTI raw monitors. JVMTI uses
ObjectWaiter for reporting on monitor usage.
That's really confusing.
http://cr.openjdk.java.net/~dholmes/8231289/webrev/src/hotspot/share/prims/jvmtiRawMonitor.cpp.frames.html
148 void JvmtiRawMonitor::SimpleExit (Thread * Self) {
128 QNode Node (Self) ;
153 QNode * w ;
162 guarantee (w ->TState == QNode::TS_ENTER, "invariant") ;
Can you fix the style problems in the lines you changed, like these?
Fixing the style in _changed_ code doesn't distract from the code
review (having the old style does!)
I'm happy to do a full style cleanup on this code later, but mixing
old and new styles just seems more distracting to me. I emulate the
existing style on the lines I change.
Do you have an RFE to clean this up?
thanks,
Coleen
This looks like a nice cleanup, and does not appear to change the
what the code does, but someone who understands the locking code
should also review it.
Thanks again for taking a look.
David
-----
Thanks,
Coleen
On 9/30/19 6:52 PM, David Holmes wrote:
ping!
Thanks,
David
On 24/09/2019 3:09 pm, David Holmes wrote:
Bug: https://bugs.openjdk.java.net/browse/JDK-8231289
webrev: http://cr.openjdk.java.net/~dholmes/8231289/webrev/
The earlier attempt to rewrite JvmtiRawMonitor as a simple wrapper
around PlatformMonitor proved not so simple and ultimately had too
many issues due to the need to support Thread.interrupt.
I'd previously stated in the bug report:
"In the worst-case I suppose we could just copy ObjectMonitor to a
new class and have JvmtiRawMonitor continue to extend that (with
some additional minor adjustments) - or even just inline it all as
needed."
but hadn't looked at it in detail. Richard Reingruber did look at
it and pointed out that it is actually quite simple - we barely use
any actual code from ObjectMonitor, mainly just the state. So
thanks Richard! :)
So this change basically copies or moves anything needed by
JvmtiRawMonitor from ObjectMonitor, breaking the connection between
the two. We also copy and simplify ObjectWaiter, turning it into a
QNode internal class. There is then a lot of cleanup that was
applied (and a lot more that could still be done):
- Removed the never implemented/used PROPER_TRANSITIONS ifdefs
- Fixed the disconnect between the types of non-JavaThreads
expected by the upper layer code and lower layer code
- cleaned up and simplified return codes
- consolidated code that is identical for JavaThreads and
non-JavaThreads (e.g. notify/notifyAll).
- removed used of TRAPS/THREAD where not appropriate and replaced
with "Thread * Self" in the style of the rest of the code
- changed recursions to be int rather than intptr_t (a "fixme" in
the ObjectMonitor code)
I have not changed the many style flaws with this code:
- Capitalized names
- extra spaces before ;
- ...
but could do so if needed. I wanted to try and keep it more obvious
that the fundamental functional code is actually unmodified.
There is one aspect that requires further explanation: the notion
of current pending monitor. The "current pending monitor" is stored
in the Thread and used by a number of introspection APIs for things
like finding monitors, doing deadlock detection, etc. The
JvmtiRawMonitor code would also set/clear itself as "current
pending monitor". Most uses of the current pending monitor
actually, explicitly or implicitly, ignore the case when the
monitor is a JvmtiRawMonitor (observed by the fact the
mon->object() query returns NULL). The exception to that is
deadlock detection where raw monitors are at least partially
accounted for. To preserve that I added the notion of "current
pending raw monitor" and updated the deadlock detection code to use
that.
The test:
test/hotspot/jtreg/vmTestbase/nsk/jvmti/RawMonitorWait/rawmnwait005/rawmnwait005.cpp
was updated because I'd noticed previously that it was the only
test that used interrupt with raw monitors, but was in fact broken:
the test thread is a daemon thread so the main thread could
terminate the VM immediately after the interrupt() call, thus you
would never know if the interruption actually worked as expected.
Testing:
- tiers 1 - 3
- vmTestbase/nsk/monitoring/ (for deadlock detection**)
- vmTestbase/nsk/jdwp
- vmTestbase/nsk/jdb/
- vmTestbase/nsk/jdi/
- vmTestbase/nsk/jvmti/
- serviceability/jvmti/
- serviceability/jdwp
- JDK: java/lang/management
** There are no existing deadlock related tests involving
JvmtiRawMonitor. It would be interesting/useful to add them to the
existing nsk/monitoring tests that cover synchronized and JNI
locking. But it's a non-trivial enhancement that I don't really
have time to do.
Thanks,
David
-----
