RFR (S): 8134631: G1DummyRegionsPerGC fires assert of assert(words <= filler_array_max_size()) failed: too big for a single object

[sangheon.kim]

Hi Tom,

On 11/11/2015 02:11 PM, Tom Benson wrote:
> Hi Sangeon,
> I do prefer this version, but I think it would be a good idea to 
> preserve the original value and restore it like you did in your first 
> version, rather than re-setting it specifically to 
> _humongous_object_threshold_in_words.
Okay, I changed it as you said.

http://cr.openjdk.java.net/~sangheki/8134631/webrev.02/

Thanks,
Sangheon


> Thanks,
> Tom
>
> On 11/11/2015 3:35 PM, sangheon.kim wrote:
>> Hi all,
>>
>> After discussion with Tom, I decided to add simple fix for this 
>> development option.
>> New change is changing filler limitation(_filler_array_max_size) when 
>> G1DummyRegionsPerGC is used and restore after the use.
>>
>> http://cr.openjdk.java.net/~sangheki/8134631/webrev.01/
>>
>> Thanks,
>> Sangheon
>>
>>
>> On 11/09/2015 04:16 PM, sangheon.kim wrote:
>>> Hi Tom,
>>>
>>> Thank you for looking at this!
>>>
>>> On 11/09/2015 01:30 PM, Tom Benson wrote:
>>>> Hi
>>>> On 11/9/2015 4:10 PM, Tom Benson wrote:
>>>>> Hi Sangheon,
>>>>> Rather than changing all the product-version uses, couldn't you 
>>>>> change _filler_array_max_size to 
>>>>> _humongous_object_threshold_in_words around the special debug-only 
>>>>> call to fill_with_object instead?
>>>>
>>>> Whoops - Meant to say... change it to HeapRegion::GrainWords. This 
>>>> (testing-only, debug) code already assumes fill_with_object can 
>>>> handle an object that large.
>>>> Tom
>>>>
>>>>>   I think it would be better to have the special-case hack there 
>>>>> in the testing code (only used when the G1DummyRegionsPerGC option 
>>>>> is specified). Then you don't need the new alternate entry point 
>>>>> and only change g1CollectedHeap.cpp.
>>> Well, we can hack here. :)
>>> And I also have a concern of whether is it worth to change codes for 
>>> debug only test option or not.
>>> However, I thought there's no reason of adding size limitation for 
>>> G1 filler.
>>> We are using non-humongous fillers for now but it would be nice to 
>>> have humongous filler as well.
>>>
>>>>>
>>>>> I'm not sure the objects allocate_dummy_regions creates really 
>>>>> MUST be humongous - its stated purpose is just to "artificially 
>>>>> expand the heap by allocating a number of dead regions."   But it 
>>>>> would take more changes to allocate_dummy_regions to allocate 
>>>>> regions and then fill with non-humongous objects, possibly not 
>>>>> worth it for this case.
>>> Yes, we would write filler with non-humongous objects but as we 
>>> already have CollectedHeap::fill_with_xxxx(), it would be better 
>>> utilizing them.
>>>
>>> Thanks,
>>> Sangheon
>>>
>>>
>>>>>
>>>>> Tom
>>>>>
>>>>> On 11/9/2015 2:28 PM, sangheon.kim wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Can I get some reviews for this change for fixing this assert?
>>>>>>
>>>>>> Previous patch(8042668: GC Support for shared heap ranges in CDS) 
>>>>>> limited _filler_array_max_size of G1CollectedHeap to be less than 
>>>>>> humongous threshold.
>>>>>> And this change led this assert as this flag is trying to fill 
>>>>>> the rest of regions with humongous object.
>>>>>>
>>>>>> As a fix, I removed the limitation to allow humongous filler and 
>>>>>> added a wrapper function, fill_with_non_humongous_objects().
>>>>>> And replace it with current fill_with_object().
>>>>>>
>>>>>> CR:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8134631
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~sangheki/8134631/webrev.00/
>>>>>> Test:
>>>>>> JPRT
>>>>>>
>>>>>> Thanks,
>>>>>> Sangheon
>>>>>>
>>>>>
>>>>
>>>
>>
>