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

[Tom Benson]

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.
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
>>>>>
>>>>
>>>
>>
>