<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Bengt,<br>
<br>
Thanks for looking over the change. Vitaly does have a point but IMO
we change all the checks to check for null or none because (a) his
point would be equally valid other places where we check the # of
marking threads, and (b) it would better to consistent so that we
don't have to search for multiple patterns if we ever need to change
this.<br>
<br>
With that said I have an idea - a new webrev will be published
shortly.<br>
<br>
JohnC<br>
<br>
<div class="moz-cite-prefix">On 1/30/2013 4:57 AM, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:510918BE.7010808@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi John,<br>
<br>
This looks good to me.<br>
<br>
I think Vitaly has a point about the fact that we just "know"
that
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
_parallel_workers == NULL is equivalent to
parallel_marking_threads() == 0.<br>
<br>
I'm not going to insist on this, but would it make sense to add
an assert to convey this information? I'm actually less worried
about the case "parallel_marking_threads() > 0 but
_parallel_workers == NULL" since that will result in a null
de-reference that can be fairly easy to debug.<br>
<br>
But maybe this assert could be added at the start of the method:<br>
<br>
assert(_parallel_workers == NULL || parallel_marking_threads()
> 0, "work gang not set up correctly"); <br>
<br>
I'm not sure we need that assert and I'm not convinced this is
the right place to have it. But my thought is that this will
detect an unexpected state that we would otherwise silently
ignore.<br>
<br>
Bengt<br>
<br>
<br>
This is probably not such a big problem if we <br>
<br>
On 1/10/13 1:47 AM, Vitaly Davidovich wrote:<br>
</div>
<blockquote
cite="mid:CAHjP37Hn=68a-xR439TKyC4LwSeaQ3WYSjE8uj+0adZ5+_KmOw@mail.gmail.com"
type="cite">
<p dir="ltr">Hi John,</p>
<p dir="ltr">Thanks for the response. Yeah, I figured it's the
same thing since it's not null iff # of workers > 0.
However, if this relationship is ever broken or perhaps the
gang can be set to null at some point even if workers > 0,
then this code will segv again. Hence I thought a null guard
is a bit better, but it was just a side comment - code looks
fine as is.</p>
<p dir="ltr">Thanks</p>
<p dir="ltr">Sent from my phone</p>
<div class="gmail_quote">On Jan 9, 2013 7:41 PM, "John
Cuthbertson" <<a moz-do-not-send="true"
href="mailto:john.cuthbertson@oracle.com">john.cuthbertson@oracle.com</a>>
wrote:<br type="attribution">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF"> Hi Vitaly,<br>
<br>
Thanks for looking over the changes. AFAICT checking if
_parallel_workers is not null is equivalent to checking
that the number of parallel marking threads is > 0. I
went with the latter check as other references to the
parallel workers work gang are guarded by it. I'm not sure
why the code was originally written that way but my guess
is that, when originally written, the marking threads
(like the concurrent refinement threads currently) were
not in a work gang.<br>
<br>
Thanks,<br>
<br>
JohnC<br>
<br>
<div>On 1/8/2013 8:37 PM, Vitaly Davidovich wrote:<br>
</div>
<blockquote type="cite">
<p dir="ltr">Hi John,</p>
<p dir="ltr">What's the advantage of checking parallel
marking thread count > 0 rather than checking if
parallel workers is not NULL? Is it clearer that way?
I'm thinking checking for NULL here (perhaps with a
comment on when NULL can happen) may be a bit more
robust in case it can be null for some other reason,
even if parallel marking thread count is > 0.</p>
<p dir="ltr">Looks good though.</p>
<p dir="ltr">Thanks</p>
<p dir="ltr">Sent from my phone</p>
<div class="gmail_quote">On Jan 8, 2013 5:14 PM, "John
Cuthbertson" <<a moz-do-not-send="true"
href="mailto:john.cuthbertson@oracle.com"
target="_blank">john.cuthbertson@oracle.com</a>>
wrote:<br type="attribution">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Everyone,<br>
<br>
Can I please have a couple of volunteers look over
the fix for this CR - the webrev can be found at: <a
moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejohnc/8005875/webrev.0/"
target="_blank">http://cr.openjdk.java.net/~johnc/8005875/webrev.0/</a><br>
<br>
Summary:<br>
One of the modules in the Kitchensink test generates
a VM_PrintThreads vm operation. The JVM crashes when
it tries to print out G1's concurrent marking worker
threads when ParallelGCThreads=0 because the work
gang has not been created. The fix is to add the
same check that's used elsewhere in G1's concurrent
marking.<br>
<br>
Testing:<br>
Kitchensink with ParallelGCThreads=0<br>
<br>
Thanks,<br>
<br>
JohnC<br>
</blockquote>
</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>