<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix"><br>
Hi Vitaly,<br>
<br>
Thanks for looking at this!<br>
<br>
Here is an updated webrev based on your feedback:<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~brutisso/8002144/webrev.01/">http://cr.openjdk.java.net/~brutisso/8002144/webrev.01/</a><br>
<br>
I applied some of your comments, but not all. I'll explain the
details with inline comments below.<br>
<br>
On 2/1/13 5:17 PM, Vitaly Davidovich wrote:<br>
</div>
<blockquote
cite="mid:CAHjP37Fhc+9z2nNbuhuWDnfM+X+zAeZv+gopFonzTtb7=ssjPw@mail.gmail.com"
type="cite">
<p dir="ltr">Hi Bengt,</p>
<p dir="ltr">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).</p>
</blockquote>
<br>
Good catch! Yes, the new-calls should be tagged with mtGC. Fixed
that.<br>
<br>
<blockquote
cite="mid:CAHjP37Fhc+9z2nNbuhuWDnfM+X+zAeZv+gopFonzTtb7=ssjPw@mail.gmail.com"
type="cite">
<p dir="ltr">4218 // Now restore saved marks, if any.<br>
4219 while (_preserved_marks.has_data()) {<br>
4220 G1PreserveMarkQueueEntry e =
_preserved_marks.remove_first();<br>
4221 e.obj->set_mark(e.mark);</p>
<p dir="ltr">Since this is operating on itself, maybe move this
into g1PreserveMarkQueue method called mark_all() or something
like that?</p>
</blockquote>
<br>
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.<br>
<br>
<blockquote
cite="mid:CAHjP37Fhc+9z2nNbuhuWDnfM+X+zAeZv+gopFonzTtb7=ssjPw@mail.gmail.com"
type="cite">
<p dir="ltr">Also, do you need the _initial_capacity field? Seems
like _current_capacity would be enough (it should also be
initialized in constructor to 0).</p>
</blockquote>
<br>
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.<br>
<br>
<blockquote
cite="mid:CAHjP37Fhc+9z2nNbuhuWDnfM+X+zAeZv+gopFonzTtb7=ssjPw@mail.gmail.com"
type="cite">
<p dir="ltr">I'd also change Preserve to Preserved in the name as
I think it's more indicative?</p>
</blockquote>
<br>
Good point. Done.<br>
<br>
<blockquote
cite="mid:CAHjP37Fhc+9z2nNbuhuWDnfM+X+zAeZv+gopFonzTtb7=ssjPw@mail.gmail.com"
type="cite">
<p dir="ltr">Finally, would it be easier to change GrowableArray
to take a growth factor as an argument so you can use something
smaller than 2?</p>
</blockquote>
<br>
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.<br>
<br>
Thanks again for looking at this!<br>
Bengt<br>
<br>
<blockquote
cite="mid:CAHjP37Fhc+9z2nNbuhuWDnfM+X+zAeZv+gopFonzTtb7=ssjPw@mail.gmail.com"
type="cite">
<p dir="ltr">Thanks</p>
<p dir="ltr">Sent from my phone</p>
<div class="gmail_quote">On Feb 1, 2013 10:11 AM, "Bengt Rutisson"
<<a moz-do-not-send="true"
href="mailto:bengt.rutisson@oracle.com">bengt.rutisson@oracle.com</a>>
wrote:<br type="attribution">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> <br>
Hi all,<br>
<br>
Could I have a couple of reviews for this change?<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ebrutisso/8002144/webrev.00/"
target="_blank">http://cr.openjdk.java.net/~brutisso/8002144/webrev.00/</a><br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
Thanks,<br>
Bengt<br>
</div>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>