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

[Vitaly Davidovich]

Hi Bengt,

Thanks for the explanation.  Regarding mtGC tagging, I actually meant
whether the new classes should extend a template class and specify the mt
type (I don't have the name offhand but I want to say it's CHeapObj<mtGC>
or something like that).

As for making GrowableArray configurable with growth factor possibly having
perf implication, what exactly do you mean? The extra space needed to store
the factor in a member? I think for your case, you can probably have
GrowableArray take a template param to specify the factor and so then
there's no space overhead - your case has a compile time constant factor as
far as I can see.  Anyway, it's fine - just thought it'd minimize the
amount of changes you need yet fix the issue you were running into.

Thanks

Sent from my phone
On Feb 4, 2013 10:33 AM, "Bengt Rutisson" <bengt.rutisson at oracle.com> wrote:

>
> 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>
> wrote:
>
>>
>> Hi all,
>>
>> Could I have a couple of reviews for this change?
>> http://cr.openjdk.java.net/~brutisso/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/a1bf3ed7/attachment.htm>