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

[sangheon.kim]

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