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
Sun Feb 10 20:12:22 UTC 2013


Hi John,

Thanks for looking at this.

I changed the guarantee to an assert. Good point.

Thanks John, John and Vitaly for the reviews. All set to push this now.

Bengt

On 2/8/13 7:07 PM, John Coomes wrote:
> 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/
> It's a nice simplification; looks good to me.
>
> My only trivial suggestion relates to the original code.  In
> G1CollectedHeap::remove_self_forwarding_pointers(), the guarantee is
> overkill (IMHO) and an assert would be good enough.
>
> 4217   guarantee(_objs_with_preserved_marks.size() ==
> 4218             _preserved_marks_of_objs.size(), "Both or none.");
>
> Since you didn't create the guarantee, your choice if you want to
> change it or leave it.
>
> -John
>
>>      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
>>
>>
>>
>>
>>
>>




More information about the hotspot-gc-dev mailing list