RFR 8240918: [REDO] Allow direct handshakes without VMThread intervention
patricio.chilano.mateo at oracle.com
Fri Apr 3 06:13:56 UTC 2020
On 4/2/20 10:15 PM, David Holmes wrote:
> Hi Patricio,
> Updates look good - thanks.
Thanks David! I'll send a v3 addressing Dan's and Martin's comments.
> On 3/04/2020 1:11 am, Patricio Chilano wrote:
>> Hi David,
>> On 4/2/20 4:45 AM, David Holmes wrote:
>>> Hi Patricio,
>>> On 2/04/2020 3:45 am, Patricio Chilano wrote:
>>>> Hi all,
>>>> Please review this redo for 8230594. Since the reason behind the
>>>> Windows timeouts turned out to be 8240902, this patch is almost
>>>> identical to the reverse of the backout modulo the rebase.
>>> Glad to see this come back! Overall changes still look good -
>>>> There is only one small fix for an issue not identified in the
>>>> original patch which I explained in the comments. To avoid this
>>>> possible issue of deleting a JavaThread that called
>>>> Thread::destroy_vm() while there are handshakers blocked in the
>>>> handshake_turn_sem semaphore, I added a check to verify the
>>>> JavaThread in question is not protected by some ThreadsList
>>>> reference before attempting the delete it. In case it is protected,
>>>> we need to at least clear the value for _thread_key to avoid
>>>> possible infinite loops at thread exit (this can happen in Solaris).
>>> Interesting problem. At some point during the shutdown sequence the
>>> thread needs to be able to execute "process any pending handshake
>>> and mark this thread as no longer handshake-capable". But it is
>>> unclear how to achieve that at this time so I'm okay with deferring
>>> this to a future RFE. Even though the VM has "terminated" this may
>>> be a custom launcher, or a hosted VM, and the main process is not
>>> necessarily going away, so it would be preferable to be able to
>>> delete the thread and free all its resources.
>> Ok, deferring this to a future RFE sounds good. I actually tried to
>> think how to safely delete the thread even in these cases but I can't
>> find a clean way without complicating too much the code. Since this
>> is a special case and only when the VM has already terminated I
>> thought this was the best approach. I see your point about the hosted
>> VM case though. But also as Coleen pointed out we are not freeing a
>> lot of other threads too so I don't think not freeing one extra
>> thread will hurt. Just to highlight that, before the main thread
>> exits I see still alive, not counting Compiler and GC threads,
>> Finalizer, Reference Handler, Signal Dispatch, Service Thread,
>> Sweeper thread, Common-Cleaner and Notification Thread. And although
>> I don't see the VMThread we are not actually deleting it. This is the
>> comment at the end of VMThread::run():
>> // We are now racing with the VM termination being carried out in
>> // another thread, so we don't "delete this". Numerous threads don't
>> // get deleted when the VM terminates
>> I'm not saying we shouldn't try to delete this thread because we are
>> not doing it with other threads though. But I think in this special
>> case keeping the code simpler is probably worth more than
>> complicating the code to free one extra thread.
>>> One nit: in the ThreadSMR code this looks really odd:
>>> static bool is_a_protected_JavaThread_with_lock(JavaThread *thread,
>>> bool skiplock = false);
>>> A "with lock" function that might not lock? I suggest just making
>>> is_a_protected_JavaThread public and calling it directly.
>> Changed. Originally I didn't want to declare
>> is_a_protected_JavaThread() public to make it clear that
>> is_a_protected_JavaThread_with_lock() is the one that should be
>> always called. But there is an assertion that we either own the
>> Threads_lock or we are at a safepoint, so if somebody tries to call
>> this in an illegal scenario it should assert.
>> Here is v2:
>> Full: http://cr.openjdk.java.net/~pchilanomate/8240918/v2/webrev/
>> Inc: http://cr.openjdk.java.net/~pchilanomate/8240918/v2/inc/webrev/
>> Thanks for looking at this David!
>>>> The only other change is that I removed the check for local polls
>>>> in Handshake::execute_direct() since after the backout,
>>>> thread-local handshakes was implemented for arm32 which was the
>>>> only remaining platform.
>>>> I tested it several times in mach5 tiers 1-6 and once in t7 and saw
>>>> no failures.
More information about the hotspot-runtime-dev