Request for review (M): 8002144: G1: large number of evacuation failures may lead to large c heap memory usage
Bengt Rutisson
bengt.rutisson at oracle.com
Fri Feb 8 16:19:40 UTC 2013
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
> <mailto: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/
> <http://cr.openjdk.java.net/%7Ebrutisso/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 <mailto: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/%7Ebrutisso/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
>> <mailto: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
>> <mailto: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/62f10797/attachment.htm>
More information about the hotspot-gc-dev
mailing list