<div dir="ltr">Hi David,<div><br></div><div><div class="gmail_extra"><div class="gmail_quote"><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">

This is a simple and efficient approach if you are happy to skip sampling when others are doing it. A couple of things about the way you implemented this though:<br>
<br>
a) the variable should be a jint. Using a jbyte is unlikely to save space because it will need to be correctly aligned on some platforms. Conversely atomic ops on unaligned subwords, if allowed, can take a performance hit. And in the worst case attempting a CAS on an unaligned jbyte could cause a crash! Also it should be marked volatile.<br>

<br>
b) there is no need to use CAS to restore the variable to zero. But you would need to use a store_fence, as done in the sync code.</blockquote><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">
<div class="im"><br>
<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">
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>
</blockquote>
<br></div>
True, try_lock and unlock are functionally equivalent but may be less performant due to the added support for blocking that you don't need.<div class="im"><br>
<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">
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/~<u></u>hiroshi/webrevs/edenchunks/<u></u>webrev.00/src/share/vm/gc_<u></u>implementation/<u></u>concurrentMarkSweep/<u></u>concurrentMarkSweepGeneration.<u></u>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>
</blockquote>
<br></div>
Yes.</blockquote><div><br></div><div><div>Thanks. We should try try_lock() first. But *if* we end up needing to go with the custom synchronization, we should improve it based on your notes.</div></div><div><br></div><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">
<div class="im"><br>
<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">
- 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_<u></u>safepoint()) would fire?<br>
</blockquote>
<br></div>
That seems reasonable. But I think you are misunderstanding what sneaking does. It isn't dangerous, you don't get multiple threads inside a critical region or anything strange like that - as long as the other threads using it will stop at safepoints of course! With monitors/mutexes a point is reached where the lock has been claimed but the new owner then blocks at a safepoint. So externally it looks like the lock is owned but the owner has not yet entered the critical region. If the VMThread needed that same lock there would be a deadlock. So the VMThread is allowed to "sneak" the lock, effectively taking ownership of it as-if the real owner blocked just before it could take ownership. The real owner can't wake up and enter the critical section until the safepoint is over and the VMThread will release the lock before it ends the safepoint. Note that any lock needed by the VMThread at a safepoint should not be used to guard critical sections in which other safepoint checks occur, else you would again get a deadlock. (And these locks are not reentrant.)<br>
</blockquote><div><br></div><div style>Thanks. If we only call try_lock() and unlock() on a mutex/monitor, then no thread will block due to a safepoint synchronization inside that mutex/monitor (as I see no ThreadBlockInVM in try_lock()). That is, we won't encounter a situation where a VM thread sneak may happen, and we don't need to worry about sneaking. Is that right?</div>
<div style><br></div><div style>Hiroshi</div><div><br></div></div></div></div></div>