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 11:39:44 UTC 2013
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
> <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/84d9ac1c/attachment.htm>
More information about the hotspot-gc-dev
mailing list