CMS parallel initial mark

[Jon Masamitsu]

On 6/10/13 12:45 PM, Thomas Schatzl wrote:
> Hi,
>
> On Fri, 2013-06-07 at 11:17 -0700, Jon Masamitsu wrote:
>> On 6/7/2013 4:23 AM, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     some notes about the second patch, the modified eden boundary
>>> sampling; in general I think the idea is good (Jon suggested that it
>>> should become default), but imho there are some issues with the
>>> implementation.
>>>
>>> On Tue, 2013-05-28 at 17:24 -0700, Hiroshi Yamauchi wrote:
>>>> Hi,
>>>>
>>>> Here's a suggested patch in a separate patch:
>>>>
>>>>     http://cr.openjdk.java.net/~hiroshi/webrevs/edenchunks/webrev.00/
>>>>
>>>> that attempts to improve on this issue by implementing an alternative
>>>> way of dividing up the eden into chunks for an increased parallelism
>>>> (or better load balancing between the GC threads) for the young gen
>>>> scan portion of the remark phase (and the now-parallelized initial
>>>> mark phase.) It uses a CAS-based mechanism that samples the object
>>>> boundaries in the eden space on the slow allocation code paths (eg. at
>>>> the TLAB refill and large object allocation times) at all times.
>>> - this patch is for DefNew/CMS only it seems. Is this correct?
>>>
>>> - in the Hotspot code base explicit != NULL checks are done for whatever
>>> reason. To keep the same style it would be nice to update the checks
>>> whether to do the sampling ("if (CMSEdenChunksRecordAlways &&
>>> _next_gen)") accordingly.
>>>
>>> (the next point is not an issue of the patch)
>>> - the check whether there is a _next_gen is odd - I do not think that
>>> DefNew works as a generation without an older generation, but I'm not
>>> sure.
>> You're correct that DefNew needs to have a _next_gen.
>>
>>> Looking at other similar code checking for _next_gen != NULL, the
>>> response is typically to update _next_gen and then asserting that
>>> _next_gen is != NULL. E.g.
>>>
>>>     if (_next_gen == NULL) {
>>>       GenCollectedHeap* gch = GenCollectedHeap::heap();
>>>       _next_gen = gch->next_gen(this);
>>>       assert(_next_gen != NULL,
>>>              "This must be the youngest gen, and not the only gen");
>>>     }
>>>
>>> Jon?
>> Yes, except in a very few situations, _next_gen should be set.
>> Exceptions I would expect is during initialization.  At the point
>> Thomas indicates and assertion would be sufficient.
>>
>> assert(_next_gen != NULL, "...");
> Thanks.
>
>>
>>> - I think CMSCollector::_eden_chunk_sampling_active should be volatile
>>> to avoid any clever compilers skipping the read in sample_eden_chunk().
>>> (Although I don't think it's possible here). See also below for avoiding
>>> that variable completely.
>> Could you explain you thinking for making it volatile.  As you have, I
>> didn't think
>> it was necessary and thought ,"If not necessary, don't do it".
> It's not necessary I think, just as a reminder to the compiler to not
> optimize that load away. I think the Atomic instruction contain the
> necessary instructions though.

Ok.  I might be a bit of a distraction to include volatile when
not necessary.
>>> (btw, throughout this code there is a lot of checking whether some
>>> NEW_C_HEAP_ARRAY returned NULL or not, and does elaborate recovery -
>>> however by default NEW_C_HEAP_ARRAY fails with an OOM anyway... - but
>>> that is certainly out of scope)
>> Thomas, could you file a CR for fixing that?  Thanks.
> CR 8016276: CMS concurrentMarkSweepGeneration contains lots of
> unnecessary allocation failure handling

Thanks.

Jon
>
>>> Same for checking whether _survivor_chunk_array is NULL - at the moment
>>> sometimes _survivor_chunk_array is checked against NULL to avoid
>>> entering code (in print_eden_and_survivor_chunk_arrays()).
>>>
>>> The condition that could be used is the one that triggers allocation of
>>> _survivor_chunk_array: (CMSParallelRemarkEnabled &&
>>> CMSParallelSurvivorRemarkEnabled) in
>>> concurrentMarkSweepGeneration.cpp:728.
>> Personally, I like a check like if (_eden_chunk_array != NULL)
>> better. I think it is more future proof.   For whatever reason we don't
>> try to dereference _eden_chunk_array if it is NULL.  An assertion
>> checking that
>>
>> (CMSParallelRemarkEnabled &&
>> CMSParallelSurvivorRemarkEnabled)
>>
>> is true would we be appropriate.
> Also good.
>
> Thomas
>
>