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