<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<div class="moz-cite-prefix">On 2/6/2015 12:15 AM, 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 </blockquote>
<br>
Good point. Not the place to be checking G1.<br>
<br>
<blockquote cite="mid:54D4781F.2070807@oracle.com" type="cite">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>
</blockquote>
<br>
I like that.<br>
<br>
Jon<br>
<br>
<blockquote cite="mid:54D4781F.2070807@oracle.com" type="cite"> <br>
Thanks,<br>
Bengt<br>
<br>
<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>