CMS parallel initial mark; vm thread sneaking in Thread::try_lock()
yamauchi at google.com
Thu Jun 13 13:46:50 PDT 2013
On Thu, Jun 13, 2013 at 4:45 AM, Thomas Schatzl
<thomas.schatzl at oracle.com>wrote:
> On Thu, 2013-06-13 at 20:33 +1000, David Holmes wrote:
> > Thomas,
> > lock-sneaking is something very specific to the VMThread to avoid
> > deadlocks at safepoints. I really don't think we want to see it promoted
> > into the API for mutexes/monitors.
> > I hadn't been paying this thread too much attention but now I'm starting
> > to think I need to.
> the situation is (and I should have asked the runtime team more
> directly, and cc'ed the thread earlier, apologies) that in CMS there is
> the need to frequently record object boundaries into a buffer for
> performance reasons.
> There is a contribution that changes this sampling from being done
> concurrently to being done every time after a tlab refill. The problem
> is that this operation involves a few reads that must be atomic; so the
> change guards that operation with a critical section. It is also not
> critical that the operation is executed always, it may be skipped some
> So the change manually implements a try_lock()/unlock() idiom using a
> CAS and a global variable.
> The suggestion that tripped this thread has been to use existing VM
> code, in particular monitors, as they seem to have the requested
> functionality. There does not seem to be need for rolling own code here.
> The code in that critical section is very short, and straight line code;
> the critical section boiler plate code is larger than that...
> Then there has been the concern about that try_lock() may be sneaked by
> the vm thread; and some suggestions to put these concerns to rest.
> in the sample_eden_chunk() method.
> So maybe could you confirm the statements made so far:
> - Monitor::try_lock()/::unlock() is equivalent to what the change does
> in this case.
> (I had a look at the unlock() code, and it's a lot more complicated than
> the equivalent code in the change. It boils down to the same behavior
> - that new sampling code is supposed to be only every executed while the
> mutator is running. Sneaking can only occur during safepoints; so it's
> use here would be "safe" in that there will be no sneaking at all. There
> is no case when the suggested assert(!
> SafepointSynchronize::is_at_safepoint()) would fire?
> Other than that some other ideas to avoid execution of the sneaking code
> were thrown into the room. The assert should be fine, but it would be
> better to avoid the possibility of sneaking directly. The latest
> suggestion involved adding a no-sneaking-variant of try_lock()
> ("try_lock_no_sneak()") that is called by the original one to keep exact
> semantics (except for the additional call).
> Nobody intends to change anything in that code without your input :)
> everyone is aware that this is tricky code with possible unintended side
> The relevant thread starts here, most of it having been cc'ed to
> Thanks for looking at this issue,
Thomas, thanks for summarizing the discussion so far.
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.
I also agree that adding the
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.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-runtime-dev