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
