Request for review (M): 8002144: G1: large number of evacuation failures may lead to large c heap memory usage

[Vitaly Davidovich]

Hi Bengt,

Couple of minor points:

You don't need to include utilities/stack.hpp in g1CollectedHeap.cpp - it's
already included in the header, which in turn is included in the inline
file, in turn included by g1CollectedHeap.cpp.

Probably update the comment of the two new stack members since it's still
talking about them being null (GrowableArray was like that).

Thanks

Sent from my phone
On Feb 6, 2013 9:47 AM, "Bengt Rutisson" <bengt.rutisson at oracle.com> wrote:

>
>
> Hi everyone,
>
> An updated webrev. Using the existing Stack<> data structure rather than
> introducing a new one. Thanks John Coomes for pointing this out!
>
> http://cr.openjdk.java.net/~**brutisso/8002144/webrev.02/<http://cr.openjdk.java.net/~brutisso/8002144/webrev.02/>
>
> With this fix I have run several hundreds of iterations of the ManyObjects
> test without any native OOME.
>
> I also noticed the PreservedMark class that is being used by PSMarkSweep.
> It would probably be good to make all collectors use this, but I think I'll
> file a separate RFE for that.
>
> Thanks,
> Bengt
>
>
>
> On 2/5/13 10:21 PM, Bengt Rutisson wrote:
>
>> On 2/5/13 10:13 PM, John Coomes wrote:
>>
>>> Bengt Rutisson (bengt.rutisson at oracle.com) wrote:
>>>
>>>> Hi John,
>>>>
>>>> Thanks for looking at this!
>>>>
>>>> On 2/4/13 10:18 PM, John Coomes wrote:
>>>>
>>>>> Bengt Rutisson (bengt.rutisson at oracle.com) wrote:
>>>>>
>>>>>> ...
>>>>>>
>>>>> It doesn't appear that G1 needs to process these in FIFO order.  If
>>>>> not and LIFO order is ok, you can use the existing Stack<> template
>>>>> which is a chunked stack with chunks a page in size (by default).  The
>>>>> other collectors use this, e.g.,
>>>>>
>>>>>     Stack<markOop, mtGC> PSScavenge::_preserved_mark_**stack;
>>>>>     Stack<oop, mtGC> PSScavenge::_preserved_oop_**stack;
>>>>>
>>>>> It would be nice to avoid a new data structure if a Stack will do.
>>>>>
>>>>> If FIFO order really is needed, consider adding a more generic data
>>>>> structure (e.g., ChunkedQueue<T>) and making G1PreserveMarkQueue a
>>>>> typedef or simple wrapper.
>>>>>
>>>> I agree that FIFO order is probably not needed. The reason I preserved
>>>> the FIFO order was mostly to reduce the risk of introducing regressions.
>>>> So, I think using the Stack data structure should be fine. I'll change
>>>> my patch to do that instead and try it out. Thanks for pointing me to
>>>> the Stack!
>>>>
>>>> I've been at JFokus all day today and I'll be there tomorrow as well.
>>>> I'll send out an updated webrev as soon as I get a chance to try it out.
>>>>
>>>> Any idea why G1 used the GrowableArray in the first place when the other
>>>> GCs use Stack?
>>>>
>>> All the GCs used GrowableArray until Stack<> was added for 6423256,
>>> and a number of GrowableArrays were converted as part of that fix.  I
>>> checked some saved email and notes but didn't find any clues as to why
>>> these weren't changed at the same time.
>>>
>>
>> Thanks for digging around to find the bug. It definitely sounds like that
>> bug addresses the same issue that I see now in G1. I'll put together a fix
>> using Stack<> instead of the GrowableArray and see if that passes my tests.
>>
>> Bengt
>>
>>
>>> -John
>>>
>>>  Without this fix I get native out of memory about every three runs of
>>>>>> the test.
>>>>>> With this fix the test has been running for several days and more
>>>>>> than 5600
>>>>>> iterations.
>>>>>>
>>>>>> The chunk size is variable but has a max limit. I use 40 entries as
>>>>>> initial
>>>>>> size since this is what the GrowableArrays used. I picked 10000 as
>>>>>> the maximum
>>>>>> size. The value 10000 can probably be tuned further, but 100000 was
>>>>>> too much (I
>>>>>> still got native OOME) and 10000 works fine.
>>>>>>
>>>>>> I have been comparing GC pause times with and without my fix for the
>>>>>> ManyObjects test. I don't see any large differences in the pause
>>>>>> times. This
>>>>>> will only affect performance for runs that have a lot of evacuation
>>>>>> failures.
>>>>>> These runs will benefit form the fact that we don't have to do as
>>>>>> much copying
>>>>>> as before, but they will also do several more mallocs compared to
>>>>>> before my
>>>>>> fix. The runs I've done indicate that this evens out. I can't see any
>>>>>> large
>>>>>> differences.
>>>>>>
>>>>> I hope we don't start caring about performance when there are many
>>>>> evacuation failures :-).
>>>>>
>>>>> -John
>>>>>
>>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130207/a19802a5/attachment.htm>