<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Bengt,<br>
<br>
<div class="moz-cite-prefix">On 06/02/15 09:15, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:54D4781F.2070807@oracle.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi Jon and Marcus,<br>
<br>
One comment inlined below...<br>
<br>
On 2/5/15 9:03 PM, Jon Masamitsu wrote:<br>
</div>
<blockquote cite="mid:54D3CCA2.2070302@oracle.com" type="cite"> <br>
On 02/05/2015 07:49 AM, Marcus Larsson wrote: <br>
<blockquote type="cite">Hi Jon, <br>
<br>
On 04/02/15 22:21, Jon Masamitsu wrote: <br>
<blockquote type="cite">Marcus, <br>
<br>
Many of the changes seem not to relate directly to the <br>
CR. For example the change "unsigned int -> uint" are <br>
the only changes is some files. Though that would be <br>
bearable in a code review, it makes more work for <br>
sustaining when they go hunting for a change that lead <br>
to a bug. Please consider integrating those under <br>
a different CR. <br>
<br>
</blockquote>
<br>
I made the cleanup changes a separate CR. <br>
<br>
Cleanup issue: <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8072621">https://bugs.openjdk.java.net/browse/JDK-8072621</a>
<br>
</blockquote>
<br>
Thanks. <br>
<br>
<blockquote type="cite"> <br>
Cleanup webrev: <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Emlarsson/8072621/webrev.00/">http://cr.openjdk.java.net/~mlarsson/8072621/webrev.00/</a>
<br>
</blockquote>
<br>
Looks good. <br>
<br>
Reviewed. <br>
<br>
<blockquote type="cite"> <br>
New refactoring webrev: <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Emlarsson/8066771/webrev.01/">http://cr.openjdk.java.net/~mlarsson/8066771/webrev.01/</a>
<br>
</blockquote>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Emlarsson/8066771/webrev.01/src/share/vm/gc_implementation/shared/vmGCOperations.cpp.frames.html">http://cr.openjdk.java.net/~mlarsson/8066771/webrev.01/src/share/vm/gc_implementation/shared/vmGCOperations.cpp.frames.html</a>
<br>
<br>
305 // G1's incremental collections are not always caused by
an allocation, which is indicated by word_size = 0. <br>
306 assert(_word_size != 0 || UseG1GC, "word_size = 0 should
only happen with G1"); <br>
<br>
<br>
Can the assert check that the GCCause is
VM_G1IncCollectionPause (I'm <br>
guessing that's what it would be) as well as UseG1GC? And
expand the <br>
message to something like <br>
<br>
err_msg("word_size is 0 and GC cause is %s", <br>
GCCause::to_string(cause)) <br>
</blockquote>
<br>
Jon, is inside vmGCOperations.cpp the correct place to be
verifying that G1 works properly? Maybe we should just remove this
assert and instead replace it with one assert each in the
constructors for VM_ParallelGCFailedAllocation and
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
VM_G1OperationWithAllocRequest. The assert in
VM_ParallelGCFailedAllocation can always check for _word_sz != 0
and the assert in VM_G1OperationWithAllocRequest can check that
_word_size != 0 || GCCause is VM_G1IncCollectionPause. <br>
<br>
Thanks,<br>
Bengt<br>
<br>
</blockquote>
<br>
I think that's a good idea, having that extra G1 logic in the G1
code, so I moved the assert to subclasses of VM_CollectForAllocation
instead. Also found that one of the G1 operations already had a
guarantee for this condition, and streamlined the assert messages
for these different cases. Not sure if I should convert the
guarantee in the G1 case into an assert, so I left it as a guarantee
to be safe. VM_G1IncCollectionPause seems to have multiple GCCauses
for when allocation size can be 0, so I don't think it's worth
making an assert for, it will now be the only
CollectForAllocation-op that allows size = 0 anyway.<br>
<br>
New webrev:<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~mlarsson/8066771/webrev.02/">http://cr.openjdk.java.net/~mlarsson/8066771/webrev.02/</a><br>
<br>
Incremental:<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~mlarsson/8066771/webrev.01-02/">http://cr.openjdk.java.net/~mlarsson/8066771/webrev.01-02/</a><br>
<br>
Thanks,<br>
Marcus<br>
<br>
<blockquote cite="mid:54D4781F.2070807@oracle.com" type="cite"> <br>
<br>
<blockquote cite="mid:54D3CCA2.2070302@oracle.com" type="cite"> <br>
Otherwise, looks good. <br>
<br>
Reviewed. <br>
<br>
Jon <br>
<br>
<br>
<blockquote type="cite"> <br>
<blockquote type="cite">Please create a CR to rename the
sub-classes of <br>
VM_CollectForAllocation with synopsis "Regularize <br>
name of VM_CollectForAllocation and subclasses". <br>
Assign it to me. <br>
</blockquote>
<br>
Done! <br>
<br>
<blockquote type="cite"> <br>
The changes themselves look good. <br>
<br>
Pending your decision of separating out the unrelated <br>
changes, consider it reviewed. <br>
<br>
Jon <br>
</blockquote>
<br>
Thank you for reviewing! <br>
Marcus <br>
<br>
<blockquote type="cite"> <br>
On 2/4/2015 7:30 AM, Marcus Larsson wrote: <br>
<blockquote type="cite">Hello again, <br>
<br>
Still looking for reviews for this old forgotten change. <br>
<br>
Thanks, <br>
Marcus <br>
<br>
On 08/12/14 12:39, Marcus Larsson wrote: <br>
<blockquote type="cite">Hi, <br>
<br>
I would like reviews for the following patch, cleaning
up and refactoring VM GC operations for failed
allocations. <br>
<br>
Summary: <br>
Different GCs have specialized VM_GC_Operations for
collecting due to allocation failure. Part of this code
is duplicated. The patch adds a VM_CollectForAllocation
class that removes this duplicated code and handles the
allocation size and result for such operations. It also
serves as a common base where tracing can easily be
added for these operations, regardless of which GC is
used. <br>
<br>
In addition to the above refactoring, the patch also
cleans up around the VM GC operations. These changes
include: <br>
* Indentation and whitespace fixes <br>
* Change 'unsigned int' to 'uint' <br>
* Change some ints to uint, where it makes more sense
<br>
(gclocker_stalled_count for example) <br>
<br>
Webrev: <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Emlarsson/8066771/webrev.00/">http://cr.openjdk.java.net/~mlarsson/8066771/webrev.00/</a>
<br>
<br>
Bug: <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8066771">https://bugs.openjdk.java.net/browse/JDK-8066771</a>
<br>
<br>
Testing: <br>
jprt, local jtreg (test/gc) <br>
<br>
Thanks, <br>
Marcus <br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>