RFR 8215047: Task terminators do not complete termination in consistent state

[zgu at redhat.com]

Thank you for the review, Yumin.

-Zhengyu

On Tue, 2019-01-29 at 07:03 +0800, yumin qi wrote:
> Zhengyu,
> 
>   It looks good to me.
>   
> Thanks
> Yumin
> 
> On Sat, Jan 26, 2019 at 12:59 AM <zgu at redhat.com> wrote:
> > Hi,
> > 
> > > > > I will think a bit more about the change while waiting for
> > the
> > > > > test
> > > > > results.
> > > > > 
> > > 
> > > Testing showed good results, no issues.
> > 
> > Thanks!
> > 
> > > 
> > > I would like to ask if I understood the changes correctly: to me
> > it
> > > seems that some of these changes are the actual fix, others seem
> > to
> > > be
> > > enhancements.
> > > 
> > > So the changes in owstTaskTerminator.cpp:68-74 seem to be the
> > actual
> > > fix for the issue - it needs to recheck _offered_termination
> > within
> > > the
> > > lock at that point because of the mentioned race.
> > I would say that this is actually an enhancement. Without it,
> > merely
> > delays the termination of this particular thread. 
> > 
> > Filed RFE: https://bugs.openjdk.java.net/browse/JDK-8217794
> > 
> > > 
> > > There are additional improvements to both terminators
> > > (owstTaskTerminator.cpp:171-174, taskqueue.cpp:220-232) that may
> > > result
> > > in potential needless additional rounds of offer_termination().
> > > I.e. before returning a result that indicates to the caller to
> > > continue
> > > doing work for another round, instead if in the meantime everyone
> > has
> > > offered termination, we are obviously done too.
> > 
> > > 
> > > If I am correct, I would strongly prefer if these changes (the
> > > enhancements) would be changed separately.
> > > 
> > > Also, the code in taskqueue.cpp:220-232 is worth an extra method
> > with
> > > a
> > > description indicating why we can return true in
> > offer_termination in
> > > that case (because obviously, if all threads are sure to have
> > offered
> > > termination, between the evaluation of the condition and trying
> > to
> > > decrement the _offer_termination variable, work has been
> > completed.
> > 
> > Okay. But I want to point out that it is not necessary a race,
> > because
> > TerminatorTerminator may decide to request an abort based on
> > external
> > conditions, which may have nothing to do with the progress of the
> > termination.
> > 
> > Updated Webrev: http://cr.openjdk.java.net/~zgu/JDK-8215047/webrev.
> > 02/i
> > ndex.html
> > 
> > Passed hotspot_gc test, will rerun all other tests.
> > 
> > > As an additional enhancement, it might be worth doing padding of
> > the
> > > ParallelTaskTerminator::_offered_termination variable.
> > > 
> > I filed a separate RFE: https://bugs.openjdk.java.net/browse/JDK-82
> > 1778
> > 5
> > 
> > 
> > -Zhengyu
> > 
> > 
> > > Thanks,
> > >   Thomas
> > > 
> > >