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

[Vitaly Davidovich]

Right (you'd get compilation error otherwise) but it's still loaded from
disk (although probably in page cache at this point, hence only minor perf
improvement).

Thanks

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

>  On 2/8/13 3:38 PM, Vitaly Davidovich wrote:
>
> Looks good.
>
>
> Thanks!
>
>  Not including the header twice may save a bit of compilation time at
> preprocessor phase :).
>
>
> Yes, except that we use include guards in the header files so they are
> only processed once even if they are included more times.
>
> Bengt
>
>  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/c3c3b9d1/attachment.htm>