RFR(XS): 8005875: G1: Kitchensink fails with ParallelGCThreads=0

[Bengt Rutisson]

Hi John,

This looks good to me.

I think Vitaly has a point about the fact that we just "know" that 
_parallel_workers == NULL is equivalent to parallel_marking_threads() == 0.

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.

But maybe this assert could be added at the start of the method:

   assert(_parallel_workers == NULL || parallel_marking_threads() > 0, 
"work gang not set up correctly");

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.

Bengt


This is probably not such a big problem if we

On 1/10/13 1:47 AM, Vitaly Davidovich wrote:
>
> Hi John,
>
> 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.
>
> Thanks
>
> Sent from my phone
>
> On Jan 9, 2013 7:41 PM, "John Cuthbertson" 
> <john.cuthbertson at oracle.com <mailto:john.cuthbertson at oracle.com>> wrote:
>
>     Hi Vitaly,
>
>     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.
>
>     Thanks,
>
>     JohnC
>
>     On 1/8/2013 8:37 PM, Vitaly Davidovich wrote:
>>
>>     Hi John,
>>
>>     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.
>>
>>     Looks good though.
>>
>>     Thanks
>>
>>     Sent from my phone
>>
>>     On Jan 8, 2013 5:14 PM, "John Cuthbertson"
>>     <john.cuthbertson at oracle.com
>>     <mailto:john.cuthbertson at oracle.com>> wrote:
>>
>>         Hi Everyone,
>>
>>         Can I please have a couple of volunteers look over the fix
>>         for this CR - the webrev can be found at:
>>         http://cr.openjdk.java.net/~johnc/8005875/webrev.0/
>>         <http://cr.openjdk.java.net/%7Ejohnc/8005875/webrev.0/>
>>
>>         Summary:
>>         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.
>>
>>         Testing:
>>         Kitchensink with ParallelGCThreads=0
>>
>>         Thanks,
>>
>>         JohnC
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130130/02ea8b75/attachment.htm>