<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jun 13, 2013 at 4:45 AM, Thomas Schatzl <span dir="ltr"><<a href="mailto:thomas.schatzl@oracle.com" target="_blank">thomas.schatzl@oracle.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi,<br>
<div><br>
On Thu, 2013-06-13 at 20:33 +1000, David Holmes wrote:<br>
> Thomas,<br>
><br>
> lock-sneaking is something very specific to the VMThread to avoid<br>
> deadlocks at safepoints. I really don't think we want to see it promoted<br>
> into the API for mutexes/monitors.<br>
><br>
> I hadn't been paying this thread too much attention but now I'm starting<br>
> to think I need to.<br>
<br>
</div>  the situation is (and I should have asked the runtime team more<br>
directly, and cc'ed the thread earlier, apologies) that in CMS there is<br>
the need to frequently record object boundaries into a buffer for<br>
performance reasons.<br>
<br>
There is a contribution that changes this sampling from being done<br>
concurrently to being done every time after a tlab refill. The problem<br>
is that this operation involves a few reads that must be atomic; so the<br>
change guards that operation with a critical section. It is also not<br>
critical that the operation is executed always, it may be skipped some<br>
times.<br>
<br>
So the change manually implements a try_lock()/unlock() idiom using a<br>
CAS and a global variable.<br>
<br>
The suggestion that tripped this thread has been to use existing VM<br>
code, in particular monitors, as they seem to have the requested<br>
functionality. There does not seem to be need for rolling own code here.<br>
<br>
The code in that critical section is very short, and straight line code;<br>
the critical section boiler plate code is larger than that...<br>
<br>
Then there has been the concern about that try_lock() may be sneaked by<br>
the vm thread; and some suggestions to put these concerns to rest.<br>
<br>
See<br>
<a href="http://cr.openjdk.java.net/~hiroshi/webrevs/edenchunks/webrev.00/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp.udiff.html" target="_blank">http://cr.openjdk.java.net/~hiroshi/webrevs/edenchunks/webrev.00/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp.udiff.html</a> ,<br>


in the sample_eden_chunk() method.<br>
<br>
So maybe could you confirm the statements made so far:<br>
- Monitor::try_lock()/::unlock() is equivalent to what the change does<br>
in this case.<br>
<br>
(I had a look at the unlock() code, and it's a lot more complicated than<br>
the equivalent code in the change. It boils down to the same behavior<br>
however?)<br>
<br>
- that new sampling code is supposed to be only every executed while the<br>
mutator is running. Sneaking can only occur during safepoints; so it's<br>
use here would be "safe" in that there will be no sneaking at all. There<br>
is no case when the suggested assert(!<br>
SafepointSynchronize::is_at_safepoint()) would fire?<br>
<br>
Other than that some other ideas to avoid execution of the sneaking code<br>
were thrown into the room. The assert should be fine, but it would be<br>
better to avoid the possibility of sneaking directly. The latest<br>
suggestion involved adding a no-sneaking-variant of try_lock()<br>
("try_lock_no_sneak()") that is called by the original one to keep exact<br>
semantics (except for the additional call).<br>
Nobody intends to change anything in that code without your input :)<br>
everyone is aware that this is tricky code with possible unintended side<br>
effects.<br>
<br>
The relevant thread starts here, most of it having been cc'ed to<br>
runtime-dev:<br>
<a href="http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2013-June/007510.html" target="_blank">http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2013-June/007510.html</a><br>
<br>
Thanks for looking at this issue,<br>
  Thomas<br>
<br>
<br></blockquote><div><br></div><div>Thomas, thanks for summarizing the discussion so far.</div><div><br></div><div>I agree that if try_lock()/unlock() does what the manual/custom synchronization does (and as efficiently), it should suffice. No need to roll one's own.</div>

<div><br></div><div style>I also agree that adding the assert(!SafepointSynchronize::is_at_safepoint()) (or assert(!Thread->current()->is_VM_thread())) may be good enough and if sneaking can be turned off for a case like this, it'd be safer.</div>
<div><br></div></div></div></div>