<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<tt>Thanks Roman!<br>
<br>
Dan<br>
<br>
</tt><br>
<div class="moz-cite-prefix">On 7/31/17 11:10 AM, Roman Kennke
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:436d8741-a387-8442-1133-cb0a3e78f8c3@redhat.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<div class="moz-cite-prefix">Looks good!<br>
<br>
Roman (not an official reviewer)<br>
<br>
PS: I've filed <a moz-do-not-send="true"
href="https://bugs.openjdk.java.net/browse/JDK-8185580">JDK-8185580:
Refactor Threads::possibly_parallel_oops_do() to use
Threads::parallel_java_threads_do()</a> to take care of the
rest for when I get back from vacation.<br>
<br>
<br>
</div>
<blockquote type="cite"
cite="mid:797922fb-4893-bfbd-8edd-b47b22f64d36@oracle.com">Latest
webrev: <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Edcubed/8185273-webrev/1/"
moz-do-not-send="true">http://cr.openjdk.java.net/~dcubed/8185273-webrev/1/</a>
<br>
<br>
Only src/share/vm/runtime/thread.cpp is changed relative to
round 0: <br>
<br>
- Revised the comment in Threads::parallel_java_threads_do. <br>
- Added the assert to Threads::assert_all_threads_claimed(). <br>
<br>
Comments, questions and feedback are welcome. <br>
<br>
Dan <br>
<br>
<br>
On 7/31/17 10:47 AM, Daniel D. Daugherty wrote: <br>
<blockquote type="cite">On 7/31/17 9:43 AM, Aleksey Shipilev
wrote: <br>
<blockquote type="cite">On 07/31/2017 05:09 PM, Daniel D.
Daugherty wrote: <br>
<blockquote type="cite">On 7/31/17 8:35 AM, Aleksey Shipilev
wrote: <br>
<blockquote type="cite">
<blockquote type="cite">Webrev URL: <a
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Edcubed/8185273-webrev/0/"
moz-do-not-send="true">http://cr.openjdk.java.net/~dcubed/8185273-webrev/0/</a>
<br>
</blockquote>
Those changes make sense, thanks. <br>
</blockquote>
Thanks for the fast review! <br>
<br>
<br>
<blockquote type="cite">It is probably worth mentioning
that Threads::parallel_java_threads_do should be in sync
with <br>
Threads::possibly_parallel_oops_do? It gets easier to
point out the symmetry: possibly_parallel_... <br>
claims all Java threads and the VMThread, so this should
also claim the VMThread. <br>
</blockquote>
We would have to be careful about how we phrase that. <br>
Threads::possibly_parallel_oops_do() claims and applies <br>
the closure to all the threads it claims. <br>
<br>
Threads::parallel_java_threads_do() is missing the claim <br>
for the VMThread (this bug), but does not apply the <br>
closure to the VMThread. <br>
</blockquote>
Yeah. It's just I had to work upwards from the gory details
explained in the comment to the actual <br>
setup for the bug to appear. I think details about
ParallelSPCleanupTask, safepoint.cpp, parity, <br>
etc. are too low-level here, and capture only the current
state of affairs. E.g. what if there are <br>
more callers to parallel_java_threads_do in future? What if
Parallel SP cleanup ceases to call it? <br>
Would the comment get outdated? Does
Threads::parallel_java_threads_do make sense without
Parallel <br>
SP cleanup? Yes, it does. Would it make sense to cherry-pick
it somewhere else with that comment as <br>
stated? Not really. <br>
<br>
AFAIU, the high-level bug is because we have to claim the
same subset of threads on all paths. From <br>
that, it becomes obvious that if
possibly_parallel_java_threads_do claims VMThread, all other
paths <br>
should claim it too. <br>
<br>
Something like this: <br>
<br>
Threads::parallel_java_threads_do(ThreadClosure* tc) { <br>
... <br>
<br>
// Thread claiming protocol requires us to claim the
same interesting threads <br>
// on all paths. Notably,
Threads::possibly_parallel_threads_do claims all <br>
// Java threads *and* the VMThread. To avoid breaking
the claiming protocol, <br>
// we have to appear to claim VMThread on this path too,
even if we would not <br>
// process the VMThread oops. <br>
VMThread* vmt = VMThread::vm_thread(); <br>
(void)vmt->claim_oops_do(true, cp); <br>
</blockquote>
<br>
I like your comment better than mine, with a slight tweak: <br>
<br>
// Thread claiming protocol requires us to claim the same
interesting threads <br>
// on all paths. Notably,
Threads::possibly_parallel_threads_do claims all <br>
// Java threads *and* the VMThread. To avoid breaking the
claiming protocol, <br>
// we have to claim VMThread on this path too, even if we
do not apply the <br>
// closure to the VMThread. <br>
<br>
<blockquote type="cite"> <br>
...and then the assert fix would seal the deal. <br>
</blockquote>
<br>
The assert diffs are applied and tested via JPRT. Please see
my <br>
other e-mail on this thread... <br>
<br>
Dan <br>
<br>
<br>
<blockquote type="cite"> <br>
Thanks, <br>
-Aleksey <br>
<br>
</blockquote>
<br>
<br>
</blockquote>
<br>
</blockquote>
<p><br>
</p>
</blockquote>
<br>
</body>
</html>