RFR(S): 8222518: Remove unnecessary caching of Parker object in java.lang.Thread
David Holmes
david.holmes at oracle.com
Fri Apr 26 05:30:08 UTC 2019
Hi Robbin,
On 25/04/2019 5:53 pm, Robbin Ehn wrote:
> Hi David,
>
> Looks good.
Thanks for the review.
> Just a question:
> It seems like we could just hold the ThreadsList over p->unpark() and
> not rely on TSM ?
Yes now it is done this way we could do the unpark while holding the TLH
and avoid relying on TSM.
> Not sure in how many places we do rely on it, but it would be nice to
> remove TSM for parkers.
TSM for Parkers was introduced by JDK-6271298 (there's a typo in the
comment in park.hpp that transposes the last 2 numbers of the bug) so I
think this is the only usage that relies on it.
> The exiting thread would set parker to NULL before removing itself from
> the threadslist and free it when it's off.
I don't think we need that complexity. It should suffice change:
JavaThread::~JavaThread() {
// JSR166 -- return the parker to the free list
Parker::Release(_parker);
_parker = NULL;
to do "delete _parker" instead.
I'll file a RFE for that.
Thanks,
David
> Thanks, Robbin
>
> On 4/24/19 9:12 AM, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8222518
>> webrev: http://cr.openjdk.java.net/~dholmes/8222518/webrev/
>>
>> The original implementation of Unsafe.unpark simply extracted the
>> JavaThread reference from the java.lang.Thread oop and if non-null
>> extracted the Parker instance from it and invoked unpark. This was
>> racy however as the native JavaThread could terminate at any time and
>> deallocate the Parker.
>>
>> That logic was fixed by JDK-6271298 which used of combination of
>> type-stable-memory "event" objects for the Parker, along with use of
>> the Threads_lock to obtain the initial reference to the Parker (from a
>> JavaThread guaranteed to be alive), together with caching the native
>> Parker pointer in a field of java.lang.Thread. Even though the native
>> thread may have terminated the Parker was still valid (even if
>> associated with a different thread) and the unpark at worst was a
>> spurious wakeup for that other thread.
>>
>> When JDK-8167108 introduced Thread Safe-Memory-Reclaimation (SMR) the
>> logic was updated to always use the safe mechanism - we grab a
>> ThreadsListHandle then check the cached field, else lookup the native
>> thread to see if it is alive and locate the Parker instance that way.
>> With SMR the caching of the Parker pointer no longer serves any
>> purpose - we no longer have a lock-free use-the-cache path versus a
>> lock-using populate-the-cache path. With SMR we've already"paid" for
>> the ability to ensure the native thread can't terminate regardless of
>> whether we lookup the field from the java.lang.Thread or the
>> JavaThread. So we can simplify the code and save a little footprint by
>> removing the cache from java.lang.Thread:
>>
>> /*
>> * JVM-private state that persists after native thread termination.
>> */
>> private long nativeParkEventPointer;
>>
>> and the supporting code from unsafe.cpp and javaClass.*pp in the JVM.
>>
>> I considered restoring the fast-path use of the cache without recourse
>> to Thread-SMR but performance measurements failed to show any benefit
>> in doing. See bug report for details.
>>
>> Thanks,
>> David
More information about the core-libs-dev
mailing list