<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<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 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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~mlarsson/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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~mlarsson/8066771/webrev.01/">http://cr.openjdk.java.net/~mlarsson/8066771/webrev.01/</a>
<br>
</blockquote>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~mlarsson/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>
<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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~mlarsson/8066771/webrev.00/">http://cr.openjdk.java.net/~mlarsson/8066771/webrev.00/</a>
<br>
<br>
Bug:
<br>
<a 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>
</body>
</html>