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

[Vitaly Davidovich]

Looks good.

Not including the header twice may save a bit of compilation time at
preprocessor phase :).

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

>
> Hi Vitaly,
>
> Thanks for looking at this again!
>
> On 2/8/13 2:14 AM, Vitaly Davidovich wrote:
>
> 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.
>
>
> Right. I'm not sure what is more hotspot-stylish. To state all your
> includes or to rely on the indirect includes. I changed as you suggested
> since it seems reasonable to be able to rely on the includes in
> g1CollectedHeap.hpp.
>
>  Probably update the comment of the two new stack members since it's
> still talking about them being null (GrowableArray was like that).
>
>
> Good catch. I thought I had done that.
>
> Updated webrev:
> http://cr.openjdk.java.net/~brutisso/8002144/webrev.03/
>
> Thanks again for looking at this!
> Bengt
>
>  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/
>>
>> 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/20130208/80a9673a/attachment.htm>