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

[Bengt Rutisson]

Hi Vitaly,

Thanks for looking at this!

Here is an updated webrev based on your feedback:
http://cr.openjdk.java.net/~brutisso/8002144/webrev.01/

I applied some of your comments, but not all. I'll explain the details 
with inline comments below.

On 2/1/13 5:17 PM, Vitaly Davidovich wrote:
>
> Hi Bengt,
>
> Does the new class need to be tagged as an mtGC type? I see that array 
> allocation does specify mtGC but not sure if the class needs/should be 
> tagged (there's some template base class for that).
>

Good catch! Yes, the new-calls should be tagged with mtGC. Fixed that.

> 4218   // Now restore saved marks, if any.
> 4219   while (_preserved_marks.has_data()) {
> 4220     G1PreserveMarkQueueEntry e = _preserved_marks.remove_first();
> 4221     e.obj->set_mark(e.mark);
>
> Since this is operating on itself, maybe move this into 
> g1PreserveMarkQueue method called mark_all() or something like that?
>

I see your point, but I would like to avoid this change for now. You are 
correct that this could technically be moved into G1PreservedMarksQueue, 
but right now the G1PreservedMarksQueue is a pure data structure without 
much logic. Moving this kind of logic in there kind of breaks the 
single-responsibility rule. I prefer to leave it in G1CollectedHeap for 
now. Ideally it should probably be located in some class between 
G1CollectedHeap and G1PreservedMarksQueue.

> Also, do you need the _initial_capacity field? Seems like 
> _current_capacity would be enough (it should also be initialized in 
> constructor to 0).
>

Actually the _initial_capacity field is needed when the data structure 
is being reused. After the first usage the queue is completely emptied. 
The next GC will reuse the same instance and that will start at the 
initial capacity again.

> I'd also change Preserve to Preserved in the name as I think it's more 
> indicative?
>

Good point. Done.

> Finally, would it be easier to change GrowableArray to take a growth 
> factor as an argument so you can use something smaller than 2?
>

Yes, that was my initial thought as well. But that has many implications 
and performance risks. I prefer to start out with this special case. If 
we see this issue in more places I think we should consider having a 
generic utility class that uses memory less aggressive than GrowableArray.

Thanks again for looking at this!
Bengt

> Thanks
>
> Sent from my phone
>
> On Feb 1, 2013 10:11 AM, "Bengt Rutisson" <bengt.rutisson at oracle.com 
> <mailto:bengt.rutisson at oracle.com>> wrote:
>
>
>     Hi all,
>
>     Could I have a couple of reviews for this change?
>     http://cr.openjdk.java.net/~brutisso/8002144/webrev.00/
>     <http://cr.openjdk.java.net/%7Ebrutisso/8002144/webrev.00/>
>
>     For the evacuation failure handling in G1 we need to store the
>     mark word for objects that we self-forward. We store these mark
>     words in two GrowableArrays. The problem is that if we have a lot
>     of objects that need to be self-forwarded we will add a lot of
>     entries to the GrowableArrays. These arrays will double in size
>     when they get full. Worst case these arrays can require more
>     consecutive free memory than is available in the OS malloc heap.
>
>     I have a test (ManyObjects) that runs out of native memory on
>     Windows 32 bit platforms because of this issue. We want to double
>     from 10MB to 20MB when we hit native out of memory.
>
>     My fix for this will reduce the risk for native out of memory.
>     Instead of doubling the size I introduce a chunked data structure
>     that will only malloc one new chunk when it gets full. This
>     requires less consecutive memory. It also has the nice side effect
>     that we don't have to copy the entries when we need more memory.
>
>     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.
>
>     Thanks,
>     Bengt
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130204/fa3c573e/attachment.htm>