RFR(XS): 8005875: G1: Kitchensink fails with ParallelGCThreads=0
john.cuthbertson at oracle.com
Wed Jan 30 17:27:12 UTC 2013
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.
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.
> 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.
>> 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.
>> 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.
>>> 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:
>>> 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.
>>> Kitchensink with ParallelGCThreads=0
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-gc-dev