<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Sorry, the new webrev:<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tamao/6976350/webrev.05/">http://cr.openjdk.java.net/~tamao/6976350/webrev.05/</a><br>
<br>
Thanks.<br>
Tao<br>
<br>
On 5/30/13 6:26 PM, Tao Mao wrote:
<blockquote cite="mid:51A7FC3A.4030601@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
Please see inline.<br>
<br>
Thanks.<br>
Tao<br>
<br>
On 5/30/13 5:53 AM, Bengt Rutisson wrote:
<blockquote cite="mid:51A74BBC.7070003@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi Tao,<br>
<br>
I think the code is a little bit confused about whether
G1MultiParGCAllocBuffer can handle an arbitary number of
AllocPriorites or just 2. All the for loops indicate that we
think we might want to change from 2 to a larger number in the
future. But the naming of a method like
words_remaining_in_retired() indicate that there can only be
one retired region. With the current implementation I think
words_remaining_in_retired() should be called something like
words_remaining_in_priority0_buffer(). <br>
</div>
</blockquote>
Done.<br>
changed to words_remaining_in_priority1_buffer()<br>
<blockquote cite="mid:51A74BBC.7070003@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
I think it would be good to make this code truly general with
respect to the number of priorities. We can then use 2 as
default, but make sure that the code works with more
priorities. To do that I think we should remove the enum
GCAllocPriority and instead have a field in
G1MultiParGCAllocBuffer that contains the maximum number of
priorities. I think that will make the code more general and
easier to read. The for loops would look like:<br>
<br>
for (int pr = 0; pr < _max_priorities; ++pr) {<br>
// do stuff<br>
}<br>
</div>
</blockquote>
It's more like code style issue. In fact, it was done this way
according to the Jon's earlier suggestion. Second, if we want to
change #buffer to 3 (it wont give more benefits to define more
than that number), we only need to add one more enum value, i.e.
GCAllocPriority3.<br>
<blockquote cite="mid:51A74BBC.7070003@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
I find the name G1MultiParGCAllocBuffer confusing since it is
not inheriting G1ParGCAllocBuffer. Maybe
G1AllocBufferContainer or something like that would make more
sense? <br>
</div>
</blockquote>
Done. <br>
Changed to G1ParGCAllocBufferContainer
<blockquote cite="mid:51A74BBC.7070003@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
I don't understand why you added initialization values to
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
GCAllocPurpose. You are only using the values that are default
in C++ anyway: 0, 1, 2. At least if you avoid adding the
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
GCAllocPurposeStart value. I think it was more readable before
your change. (The same argument holds for GCAllocPriority, but
I prefer to remove that enum all together as I described
above.)<br>
</div>
</blockquote>
See above.<br>
<blockquote cite="mid:51A74BBC.7070003@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
Have you considered moving the _retired field from
G1ParGCAllocBuffer to ParGCAllocBuffer instead of making the
retire() method virtual? (I do think your change to virtual is
needed in the current code, so good catch! But I think it
might make sense to have the logic of
G1ParGCAllocBuffer::retire() in ParGCAllocBuffer::retire()
instead.)<br>
</div>
</blockquote>
In G1ParGCAllocBuffer, we need the field _retired to handle buffer
allocation failure. This is handled differently for other
collectors. For example,
ParScanThreadState::alloc_in_to_space_slow in ParNew. Thus, moving
the _retired field up to its super class will involve additional
efforts. This is supposed to be investigated in another CR
JDK-7127700.<br>
<blockquote cite="mid:51A74BBC.7070003@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
A couple of minor things:<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
1800 if (finish_undo != true) ShouldNotReachHere();<br>
<br>
should be:<br>
<br>
1800 if (!finish_undo) ShouldNotReachHere();<br>
</div>
</blockquote>
Done.<br>
<blockquote cite="mid:51A74BBC.7070003@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
Please add spaces before and after "=" here:<br>
1804 size_t result=0;<br>
</div>
</blockquote>
Done.<br>
<blockquote cite="mid:51A74BBC.7070003@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
There are two spaces after "=" here:<br>
1812 G1ParGCAllocBuffer* retired =
_priority_buffer[GCAllocPriority1];<br>
</div>
</blockquote>
Done.<br>
<blockquote cite="mid:51A74BBC.7070003@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
Also, in g1CollectedHeap.hpp you have updated the copyright
year but not in parGCAllocBuffer.hpp. As you know we in the GC
team have agreed not to update the copyright year. If you
absolutely want to do it I think you should do it the same way
in all files.<br>
</div>
</blockquote>
Done.<br>
<blockquote cite="mid:51A74BBC.7070003@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
Thanks,<br>
Bengt<br>
<br>
On 5/24/13 1:31 AM, Tao Mao wrote:<br>
</div>
<blockquote cite="mid:519EA6EA.1080308@oracle.com" type="cite">Can
I have a couple of reviewers please? <br>
<br>
Thank you. <br>
Tao <br>
<br>
On 5/20/13 5:11 PM, Tao Mao wrote: <br>
<blockquote type="cite">Hi all, <br>
<br>
a new webrev <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/6976350/webrev.04/">http://cr.openjdk.java.net/~tamao/6976350/webrev.04/</a>
<br>
<br>
diff: <br>
(1) John Cuthbertson and I figured out the way to handle
"retire an old buffer, allocate and set a new one" and it
can possibly make the usage of allocation buffer a little
more efficient. <br>
(2) Make the assertion as John suggested and provide some
harness (i.e. making retire() a virtual function) to cope
with it. <br>
<br>
Thanks. <br>
Tao <br>
<br>
On 5/15/13 10:58 PM, John Cuthbertson wrote: <br>
<blockquote type="cite">Hi Tao, <br>
<br>
This looks excellent. One minor question: does it make
sense to assert that each buffer has been retired? It
might save a missed call to PSS::retire_alloc_buffers.
I'll leave the decision to you. Ship it. <br>
<br>
JohnC <br>
<br>
<br>
On 5/14/2013 3:06 PM, Tao Mao wrote: <br>
<blockquote type="cite">To the open list: <br>
<br>
new webrev: <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/6976350/webrev.03/">http://cr.openjdk.java.net/~tamao/6976350/webrev.03/</a>
<br>
<br>
I took suggestion from many reviewers into consideration
and came up with the current cleaner solution. <br>
<br>
Thank you. <br>
Tao <br>
<br>
<br>
On 5/14/13 2:26 PM, Jon Masamitsu wrote: <br>
<blockquote type="cite">What's the status of this
review? <br>
<br>
The last mail I could find in my mail boxes was a
comment <br>
from Thomas. <br>
<br>
Jon <br>
<br>
On 1/28/13 12:21 PM, Tao Mao wrote: <br>
<blockquote type="cite">6976350 G1: deal with
fragmentation while copying objects during GC <br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://jbs.oracle.com/bugs/browse/JDK-6976350">https://jbs.oracle.com/bugs/browse/JDK-6976350</a>
<br>
<br>
webrev: <br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/6976350/webrev.00/">http://cr.openjdk.java.net/~tamao/6976350/webrev.00/</a>
<br>
<br>
changeset: <br>
Basically, we want to reuse more of par-allocation
buffers instead of retiring it immediately when it
encounters an object larger than its remaining part.
<br>
<br>
(1) instead of previously using one allocation
buffer per GC purpose, we use N(=2) buffers per GC
purpose and modify the corresponding code. The
changeset would easily scale up to whatever N
(though Tony Printezis suggests 2, or 3 may be good
enough) <br>
<br>
*(2) Two places of cleanup:
allocate_during_gc_slow() is removed due to its
never being called. <br>
access
modifier (public) before trim_queue() is redundant.
<br>
<br>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
</body>
</html>