RFR 8240918: [REDO] Allow direct handshakes without VMThread intervention
david.holmes at oracle.com
Fri Apr 3 01:15:12 UTC 2020
Updates look good - thanks.
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 - naturally.
>>> 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
>>> 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