<p dir="ltr">Looks good.</p>
<p dir="ltr">Not including the header twice may save a bit of compilation time at preprocessor phase :).</p>
<p dir="ltr">Sent from my phone</p>
<div class="gmail_quote">On Feb 8, 2013 6:39 AM, "Bengt Rutisson" <<a 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">
<div><br>
Hi Vitaly, <br>
<br>
Thanks for looking at this again!<br>
<br>
On 2/8/13 2:14 AM, Vitaly Davidovich wrote:<br>
</div>
<blockquote type="cite">
<p dir="ltr">Hi Bengt,</p>
<p dir="ltr"> Couple of minor points:</p>
<p dir="ltr">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.</p>
</blockquote>
<br>
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.<br>
<br>
<blockquote type="cite">
<p dir="ltr">Probably update the comment of the two new stack
members since it's still talking about them being null
(GrowableArray was like that).</p>
</blockquote>
<br>
Good catch. I thought I had done that.<br>
<br>
Updated webrev:<br>
<a href="http://cr.openjdk.java.net/~brutisso/8002144/webrev.03/" target="_blank">http://cr.openjdk.java.net/~brutisso/8002144/webrev.03/</a><br>
<br>
Thanks again for looking at this!<br>
Bengt<br>
<br>
<blockquote type="cite">
<p dir="ltr">Thanks</p>
<p dir="ltr">Sent from my phone</p>
<div class="gmail_quote">On Feb 6, 2013 9:47 AM, "Bengt Rutisson"
<<a href="mailto:bengt.rutisson@oracle.com" target="_blank">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">
<br>
<br>
Hi everyone,<br>
<br>
An updated webrev. Using the existing Stack<> data
structure rather than introducing a new one. Thanks John
Coomes for pointing this out!<br>
<br>
<a href="http://cr.openjdk.java.net/%7Ebrutisso/8002144/webrev.02/" target="_blank">http://cr.openjdk.java.net/~brutisso/8002144/webrev.02/</a><br>
<br>
With this fix I have run several hundreds of iterations of the
ManyObjects test without any native OOME.<br>
<br>
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.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<br>
<br>
On 2/5/13 10:21 PM, Bengt Rutisson wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 2/5/13 10:13 PM, John Coomes wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Bengt Rutisson (<a href="mailto:bengt.rutisson@oracle.com" target="_blank">bengt.rutisson@oracle.com</a>)
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi John,<br>
<br>
Thanks for looking at this!<br>
<br>
On 2/4/13 10:18 PM, John Coomes wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Bengt Rutisson (<a href="mailto:bengt.rutisson@oracle.com" target="_blank">bengt.rutisson@oracle.com</a>)
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
...<br>
</blockquote>
It doesn't appear that G1 needs to process these in
FIFO order. If<br>
not and LIFO order is ok, you can use the existing
Stack<> template<br>
which is a chunked stack with chunks a page in size
(by default). The<br>
other collectors use this, e.g.,<br>
<br>
Stack<markOop, mtGC>
PSScavenge::_preserved_mark_stack;<br>
Stack<oop, mtGC> PSScavenge::_preserved_oop_stack;<br>
<br>
It would be nice to avoid a new data structure if a
Stack will do.<br>
<br>
If FIFO order really is needed, consider adding a more
generic data<br>
structure (e.g., ChunkedQueue<T>) and making
G1PreserveMarkQueue a<br>
typedef or simple wrapper.<br>
</blockquote>
I agree that FIFO order is probably not needed. The
reason I preserved<br>
the FIFO order was mostly to reduce the risk of
introducing regressions.<br>
So, I think using the Stack data structure should be
fine. I'll change<br>
my patch to do that instead and try it out. Thanks for
pointing me to<br>
the Stack!<br>
<br>
I've been at JFokus all day today and I'll be there
tomorrow as well.<br>
I'll send out an updated webrev as soon as I get a
chance to try it out.<br>
<br>
Any idea why G1 used the GrowableArray in the first
place when the other<br>
GCs use Stack?<br>
</blockquote>
All the GCs used GrowableArray until Stack<> was
added for 6423256,<br>
and a number of GrowableArrays were converted as part of
that fix. I<br>
checked some saved email and notes but didn't find any
clues as to why<br>
these weren't changed at the same time.<br>
</blockquote>
<br>
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.<br>
<br>
Bengt<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
-John<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Without this fix I get native out of memory about
every three runs of the test.<br>
With this fix the test has been running for several
days and more than 5600<br>
iterations.<br>
<br>
The chunk size is variable but has a max limit. I
use 40 entries as initial<br>
size since this is what the GrowableArrays used. I
picked 10000 as the maximum<br>
size. The value 10000 can probably be tuned further,
but 100000 was too much (I<br>
still got native OOME) and 10000 works fine.<br>
<br>
I have been comparing GC pause times with and
without my fix for the<br>
ManyObjects test. I don't see any large differences
in the pause times. This<br>
will only affect performance for runs that have a
lot of evacuation failures.<br>
These runs will benefit form the fact that we don't
have to do as much copying<br>
as before, but they will also do several more
mallocs compared to before my<br>
fix. The runs I've done indicate that this evens
out. I can't see any large<br>
differences.<br>
</blockquote>
I hope we don't start caring about performance when
there are many<br>
evacuation failures :-).<br>
<br>
-John<br>
</blockquote>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</div>
</blockquote>
<br>
</div>
</blockquote></div>