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.java.net/pipermail/hotspot-gc-dev/attachments/20130208/84d9ac1c/attachment.htm>


More information about the hotspot-gc-dev mailing list