<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Kim,<br>
<br>
Thanks for reviewing this.<br>
<br>
<div class="moz-cite-prefix">On 5/8/19 9:42 PM, Kim Barrett wrote:<br>
</div>
<blockquote type="cite"
cite="mid:3B084725-AAEC-4D78-9AC7-A16492E01613@oracle.com">
<blockquote type="cite">
<pre wrap="">On Apr 30, 2019, at 2:06 AM, <a class="moz-txt-link-abbreviated" href="mailto:sangheon.kim@oracle.com">sangheon.kim@oracle.com</a> wrote:
Hi all,
I got some comments from Thomas Schatzl, mostly about comment and assert changes. He also suggested to investigate combining current 2 steps of handling retaining survivor region into 1 step, so I filed JDK-8223106.
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Esangheki/8220089/webrev.2/">http://cr.openjdk.java.net/~sangheki/8220089/webrev.2/</a>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Esangheki/8220089/webrev.2_inc/">http://cr.openjdk.java.net/~sangheki/8220089/webrev.2_inc/</a>
Testing: hs-tier1
</pre>
</blockquote>
<pre wrap="">------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1AllocRegion.cpp
367 bool SurvivorGCAllocRegion::should_retain(HeapRegion* region) {
368 if (region == NULL || region->free() < MinTLABSize) {
369 return false;
370 }
371 return true;
372 }
Why does should_retain accept a NULL region?</pre>
</blockquote>
Removed NULL check at should_retain() and added NULL check before
calling should_retain.<br>
G1AllocRegion::release() which used as an input of should_retain()
may return NULL.<br>
<br>
<blockquote type="cite"
cite="mid:3B084725-AAEC-4D78-9AC7-A16492E01613@oracle.com">
<pre wrap="">
------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1AllocRegion.hpp
270 SurvivorGCAllocRegion(G1EvacStats* stats)
271 : G1GCAllocRegion("Survivor GC Alloc Region", false /* bot_updates */, stats, InCSetState::Young) { }
272 static bool should_retain(HeapRegion* region);
Maybe add a blank line before should_retain declaration.</pre>
</blockquote>
Done.<br>
<br>
<blockquote type="cite"
cite="mid:3B084725-AAEC-4D78-9AC7-A16492E01613@oracle.com">
<pre wrap="">
------------------------------------------------------------------------------
[discussed this offline with Sangheon and Thomas; mentioning it here
in case anyone else is following email.]</pre>
</blockquote>
I discussed with Kim and Thomas. And we decided not to pursue this
nTAMS idea.<br>
nTAMS is used in many locations and currently it is updated mostly
at the start/end of marking.<br>
And changing the value conditionally seems riskier. So decided to
use current approach which is more localized.<br>
<br>
<blockquote type="cite"
cite="mid:3B084725-AAEC-4D78-9AC7-A16492E01613@oracle.com">
<pre wrap="">
"root regions" representation was changed from an array of HeapRegion*
to an array of HeapWord*, where the values are the captured "top"
values from the corresponding regions. This was done because later
allocation in survivor regions would change top, and we don't want
that changed value.
It seems like it might be easier and simpler to retain the existing
representation, but capture the desired "top" value in NTAMS. Then in
scan_root_region, for survivor regions scan from bottom to NTAMS. That
means scan_root_region is different for old/survivor regions. But
that's kind of already true; it's a bit of a hack that the current
scan from NTAMS to top works for both.
This also requires looking at and possibly adjusting other uses of NTAMS.
------------------------------------------------------------------------------
Consider renaming
G1CollectedHeap::survivor_convert_to_eden() => convert_survivor_to_eden() ?</pre>
</blockquote>
Done.<br>
<br>
<blockquote type="cite"
cite="mid:3B084725-AAEC-4D78-9AC7-A16492E01613@oracle.com">
<pre wrap="">
------------------------------------------------------------------------------
I dislike the "retained" region nomenclature for survivors. I didn't
notice an explanation of it, and it's not obvious why that name was
chosen or how it relates to how these regions are used. But I don't
have a good suggestion for an alternative yet. It seems like that name
was copied from MutatorAllocRegion, but I'm not sure the survivor case
is really that similar.</pre>
</blockquote>
Yeah, I agree that it is not that similar. But as I don't have
better name, I copied from mutator case.<br>
Initially I named 'reuse' but seems not better than 'retain'.<br>
Unless there is a better naming suggestion, I would like to proceed
as is.<br>
<br>
<blockquote type="cite"
cite="mid:3B084725-AAEC-4D78-9AC7-A16492E01613@oracle.com">
<pre wrap="">
I also dislike some of the accounting kludges around retained regions,
such as for used_bytes. Maybe the above discussed use of NTAMS might
provide information that could simplify the accounting?</pre>
</blockquote>
N/A.<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8220089/webrev.3">http://cr.openjdk.java.net/~sangheki/8220089/webrev.3</a><br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8220089/webrev.3_inc/">http://cr.openjdk.java.net/~sangheki/8220089/webrev.3_inc/</a><br>
Testing: hs-tier1 ~ 5, tier7-gc<br>
<br>
Thanks,<br>
Sangheon<br>
<br>
</body>
</html>