<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<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/~dcubed/8185273-webrev/1/">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/~dcubed/8185273-webrev/0/">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>
</body>
</html>