RFR 8240918: [REDO] Allow direct handshakes without VMThread intervention

Fri Apr 3 09:05:02 UTC 2020

Hi Patricio,

thanks for the update. Much better this way.
Atomic::add and dec also provide specifying explicit atomic_memory_order, but I guess we don't need to optimize performance here?
The comments are helpful. Thanks.

Best regards,
Martin

> -----Original Message-----
> From: Patricio Chilano <patricio.chilano.mateo at oracle.com>
> Sent: Freitag, 3. April 2020 10:01
> To: Doerr, Martin <martin.doerr at sap.com>; Robbin Ehn
> <robbin.ehn at oracle.com>; hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR 8240918: [REDO] Allow direct handshakes without
> VMThread intervention
> 
> Hi Martin,
> 
> On 4/2/20 5:24 PM, Doerr, Martin wrote:
> > Hi Patricio,
> >
> > right, you got my point. Sorry for not explaining it in greater detail at the
> beginning. I was busy with other things.
> >
> > There should be a memory barrier ordering read of _pending_threads
> (which is observed as 0) and whatever gets done when returning from
> execute_direct.
> > I don't like relying on barriers which are hidden somewhere and may be
> subject to change.
> Yes, I agree with that. I added an explicit acquire fence at the end of
> Handshake::execute_direct() with a comment. Thanks for noticing it.
> 
> > I'd also prefer explicit barriers on the writers side over the implicit ones
> from Atomic::add, because explicit barriers with comments help a lot
> understanding the code.
> > (Not sure if you want to change that. Just my opinion.)
> I added a comment on top of Atomic::dec() on why we need a memory
> barrier there and the fact that we are already using a full memory fence.
> If you still think I should add an explicit barrier for code readability
> I can add it though.
> 
> 
> Here is v3 which also addresses Dan's comments.
> 
> Full:
> http://cr.openjdk.java.net/~pchilanomate/8240918/v3/webrev/
> Inc:
> http://cr.openjdk.java.net/~pchilanomate/8240918/v3/inc/webrev/
> 
> 
> Thanks!
> Patricio
> > Best regards,
> > Martin
> >
> >
> >> -----Original Message-----
> >> From: Patricio Chilano <patricio.chilano.mateo at oracle.com>
> >> Sent: Donnerstag, 2. April 2020 19:56
> >> To: Robbin Ehn <robbin.ehn at oracle.com>; Doerr, Martin
> >> <martin.doerr at sap.com>; hotspot-runtime-dev at openjdk.java.net
> >> Subject: Re: RFR 8240918: [REDO] Allow direct handshakes without
> >> VMThread intervention
> >>
> >>
> >> On 4/2/20 1:16 PM, Patricio Chilano wrote:
> >>> Hi Martin,
> >>>
> >>> Thanks for looking at this.
> >>>
> >>> On 4/2/20 10:15 AM, Robbin Ehn wrote:
> >>>> Hi Martin,
> >>>>
> >>>> On 2020-04-02 14:55, Doerr, Martin wrote:
> >>>>> Hi all,
> >>>>>
> >>>>> first of all, thanks, Patricio, for doing this.
> >>>>>
> >>>>> One question:
> >>>>> _pending_threads is used for synchronizing threads.
> >>>>> Don't we need release / acquire semantics?
> >>>>> I guess otherwise handshaker can read stale data from handshakee
> >>>>> after observing is_completed.
> >>>>> Or did I miss anything?
> >>>> Reading stale should be fine in this case, since the counter goes
> >>>> from n->0, thus you can only get false negatives from "bool
> >>>> is_completed()" ?
> >>> Right, as Robbin points out at most we will just have to loop again.
> >>> Also, when the handshakee processes the handshake, right after
> >>> updating _pending_threads, it will either signal the _processing_sem
> >>> if there are no direct operations, or it will signal the
> >>> _handshake_turn_sem, so that will provide the release already. The
> >>> next read in the loop should read the updated value. Are you okay with
> >>> that?
> >> I realize you might be thinking of a different case now. You mean the
> >> handshaker could execute loads from code after
> >> Handshake::execute_direct() while it hasn't yet read _pending_threads
> >> from is_completed()? Although there is a load_acquire() in
> >> ~ThreadsListHandle() that will prevent those issues you might argue that
> >> we can't rely on code which might change.
> >> I think (not counting that load_acquire from ~ThreadsListHandle) the
> >> issue could appear for example if you created a ThreadsListHandle first,
> >> then called Handshake::execute_direct(), and when returning you try to
> >> read some field from the handshakee (you might want to assert
> something
> >> for example). If there wasn't any ThreadsListHandle created before
> >> calling execute_direct() I think you are not allowed to access other
> >> JavaThread's data anyway since the thread might have exited already,
> >> unless you create a ThreadsListHandle which will already provide the
> >> acquire. So I think you are right and we might need a fence before
> >> returning from execute_direct() or add a comment that we are relying on
> >> the load_acquire from ~ThreadsListHandle(). Is that what you were
> thinking?
> >>
> >> Thanks,
> >> Patricio
> >>> Thanks!
> >>>
> >>> Patricio
> >>>> Thanks, Robbin
> >>>>
> >>>>> Best regards,
> >>>>> Martin
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: hotspot-runtime-dev <hotspot-runtime-dev-
> >>>>>> bounces at openjdk.java.net> On Behalf Of Robbin Ehn
> >>>>>> Sent: Donnerstag, 2. April 2020 13:36
> >>>>>> To: Patricio Chilano <patricio.chilano.mateo at oracle.com>;
> >>>>>> hotspot-runtime-
> >>>>>> dev at openjdk.java.net
> >>>>>> Subject: Re: RFR 8240918: [REDO] Allow direct handshakes without
> >>>>>> VMThread intervention
> >>>>>>
> >>>>>> Hi Patricio,
> >>>>>>
> >>>>>> Thanks for fixing, looks good.
> >>>>>>
> >>>>>> /Robbin
> >>>>>>
> >>>>>> On 2020-04-01 19:45, 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.
> >>>>>>>
> >>>>>>> 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).
> >>>>>>>
> >>>>>>> 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.
> >>>>>>>
> >>>>>>> Bug:
> >>>>>>>       https://bugs.openjdk.java.net/browse/JDK-8240918
> >>>>>>>
> >>>>>>> Webrev:
> >>>>>>> http://cr.openjdk.java.net/~pchilanomate/8240918/v1/webrev/
> >>>>>>>
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Patricio