Quick follow up here then I'll respond to updated webrev ...
On 21/10/2015 12:51 AM, Daniel D. Daugherty wrote:
On 10/20/15, 1:53 AM, David Holmes wrote:
Hi Dan,
Great find in getting to the bottom of this one!
Thanks! And thanks for the fast review!
Also, welcome back from vacation... hope you had a blast.
Thanks - I did! :)
On 20/10/2015 1:02 PM, Daniel D. Daugherty wrote:
Greetings,
I have a fix for a long standing race between the lock-free
ObjectMonitor
verification code and the normal (locked) ObjectMonitor block allocation
code path. For this fix, I would like at least a Runtime team reviewer
and a Serviceability team reviewer. Thanks!
JDK-8047212 runtime/ParallelClassLoading/bootstrap/random/inner-complex
assert(ObjectSynchronizer::verify_objmon_isinpool(inf))
failed:
monitor is invalid
https://bugs.openjdk.java.net/browse/JDK-8047212
Webrev URL:
http://cr.openjdk.java.net/~dcubed/8047212-webrev/0-jdk9-hs-rt/
src/share/vm/runtime/synchronizer.cpp
I would have just used OrderAccess::storeStore() as per my comment in
the CR.
Saw your comment, but I already had the current fix in my stress
testing cycle so I went with it...
I could use OrderAccess::storestore(), but I think I'm more fond
of "OrderAccess::release_store_ptr(&gBlockList, temp)". To me the
semantics are more clear:
release previous stores before storing a new value in this variable
I _think_ OrderAccess::release_store_ptr() is also a little less
heavyweight than OrderAccess::storestore(), but I'd have to re-read
everything in orderAccess.?pp to nail that down for sure.
But as Carsten says a "release" should be paired with an "acquire".
Which suggests that in the other code that reads these variables we
also need either the load_acquire() or a loadLoad() (if using
storeStore()).**
Yes, Carsten is right and I was modeling after other ObjectMonitor
code that doesn't do this quite right. I'll fix this to use
load_acquire().
** This symmetry is largely missing in our lock-free code, and I think
we've been relying on "volatile" to act as a compiler barrier. :(
Hey it seems to work! (famous last words)
Well we're gradually getting around to cleaning it up. :)
src/share/vm/runtime/vmStructs.cpp
Can you not just define volatile_static_field?
Yes, I went that way originally and then I changed my mind to
avoid colliding with the other format. See below.
Why does the ptr aspect need to come into it? Also "static pointer
volatile field" sounds really odd, it is a static, volatile field that
happens to be a pointer-type.
It's meant to be odd because it follows the actual decl:
static ObjectMonitor * volatile gBlockList;
So "static pointer volatile field" is exactly what I have:
static ObjectMonitor * volatile gBlockList;
=> (static ObjectMonitor *) volatile gBlockList;
i.e. I have a static ObjectMonitor pointer that is volatile
Compared to these two forms:
static volatile ObjectMonitor * gBlockList;
static ObjectMonitor volatile * gBlockList;
=> static (volatile ObjectMonitor) * gBlockList;
=> static (ObjectMonitor volatile) * gBlockList;
i.e. I have a static pointer to a volatile ObjectMonitor.
Hopefully, this makes my reasons a bit more clear...
Not really :) Yes there is a difference between a "volatile pointer to
Foo" and "pointer to a volatile Foo", but for the sake of vmstructs we
don't really seem to need to care about that. The two questions are:
- is the field/variable static
- is the field/variable volatile
Cheers,
David
Dan
Thanks,
David
Testing: Aurora Adhoc RT-SVC nightly batch
4 inner-complex fastdebug parallel runs for 4+ days and
600K iterations without seeing this failure; the experiment
is still running; final results to be reported at the end
of the review cycle
JPRT -testset hotspot
This fix:
- makes ObjectMonitor::gBlockList volatile
- uses "OrderAccess::release_store_ptr(&gBlockList, temp)" to
make sure the new block updates _happen before_ gBlockList is
changed to refer to the new block
- add SA support for a "static pointer volatile" field like:
static ObjectMonitor * volatile gBlockList;
See the following link for a nice description of what "volatile"
means in the different positions on a variable/parameter decl line:
http://www.embedded.com/electronics-blogs/beginner-s-corner/4023801/Introduction-to-the-Volatile-Keyword
Thanks, in advance, for any comments, questions or suggestions.
Dan