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

[John Cuthbertson]

Hi Bengt,

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.

With that said I have an idea - a new webrev will be published shortly.

JohnC

On 1/30/2013 4:57 AM, Bengt Rutisson wrote:
>
> 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/b78102c4/attachment.htm>