<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hi Thomas,<div class=""><br class=""></div><div class="">Thanks a lot for the review!</div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Aug 7, 2017, at 7:52 AM, Thomas Schatzl <<a href="mailto:thomas.schatzl@oracle.com" class="">thomas.schatzl@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">Hi,<br class=""><br class="">On Thu, 2017-08-03 at 17:15 -0700, Jiangli Zhou wrote:<br class=""><blockquote type="cite" class="">Here are the updated webrevs.<br class=""><br class=""><a href="http://cr.openjdk.java.net/~jiangli/8179302/webrev.hotspot.02/" class="">http://cr.openjdk.java.net/~jiangli/8179302/webrev.hotspot.02/</a><br class="">http://cr.openjdk.java.net/~jiangli/8179302/webrev.whitebox.02/<br class=""><br class="">Changes in the updated webrevs include:<br class="">Merge with Ioi’s recent shared space auto-sizing change (8072061)<br class="">Addressed all feedbacks from Ioi and Coleen (Thanks for detailed<br class="">review!)<br class=""></blockquote><br class="">- the comment in g1Allocator.hpp:326 needs to be updated. I would merge<br class="">the information from the _open member here. I.e. define what "open" and<br class="">"closed" archive are.<br class=""></div></blockquote><div><br class=""></div><div>Good catch. I updated the comments as the following:</div><div><br class=""></div><div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(83, 48, 225);" class="">// G1ArchiveAllocator is used to allocate memory in archive</div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(83, 48, 225);" class="">// regions. Such regions are not scavenged nor compacted by GC.</div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(83, 48, 225);" class="">// There are two types of archive regions, which are</div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(83, 48, 225);" class="">// differ in the kind of references allowed for the contained objects:</div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(83, 48, 225);" class="">//</div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(83, 48, 225);" class="">// - 'Closed' archive region contain no references outside of archive</div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(83, 48, 225);" class="">// regions. The region is immutable by GC. GC does not mark object</div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(83, 48, 225);" class="">// header in 'closed' archive region. </div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(83, 48, 225);" class="">// - An 'open' archive region may contain references pointing to</div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(83, 48, 225);" class="">// non-archive heap region. GC can adjust pointers and mark object</div><div style="margin: 0px; font-size: 13px; font-family: Menlo; color: rgb(83, 48, 225);" class="">// header in 'open' archive region.</div></div><br class=""><blockquote type="cite" class=""><div class=""><br class="">- g1Allocator.inline.hpp:66-72: formatting - parameters should be<br class="">aligned below each other<br class=""></div></blockquote><div><br class=""></div>Fixed.</div><div><br class=""><blockquote type="cite" class=""><div class=""><br class="">- g1CollectedHeap.cpp:665/6: formatting - parameters should be aligned<br class="">below each other<br class=""></div></blockquote><div><br class=""></div>Fixed.</div><div><br class=""><blockquote type="cite" class=""><div class=""><br class="">- g1CollectedHeap.cpp:750 + 756: maybe make a static method to avoid<br class="">repetition here.<br class=""></div></blockquote><div><br class=""></div><div>I changed the code to be following. New static function is a little overkill since the usage is very limited. :-)</div><div><br class=""></div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""> HeapWord* top;</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""> HeapRegion* next_region;</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""> <span style="font-variant-ligatures: no-common-ligatures; color: #ce7924" class="">if</span> (curr_region != last_region) {</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""> top = curr_region->end();</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""> next_region = _hrm.next_region_in_heap(curr_region);</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""> } <span style="font-variant-ligatures: no-common-ligatures; color: #ce7924" class="">else</span> {</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""> top = last_address + <span style="font-variant-ligatures: no-common-ligatures; color: #c33720" class="">1</span>;</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""> next_region = <span style="font-variant-ligatures: no-common-ligatures; color: #c33720" class="">NULL</span>;</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""> }</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""> curr_region->set_top(top);</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""> curr_region->set_first_dead(top);</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""> curr_region->set_end_of_live(top);</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""> curr_region = next_region;</div><div style="margin: 0px; font-size: 13px; font-family: Menlo;" class=""> }</div><blockquote type="cite" class=""><div class=""><br class="">- G1NoteEndOfConcMarkClosure::doHeapRegion(): would it be too much work<br class="">to make an extra CR out of this change? It is a change that fixes an<br class="">existing bug unrelated to this change after all (not doing remembered<br class="">set cleanup work for archive regions).</div></blockquote><div><br class=""></div>Using separate CR to track this sounds good. I just created <a class="issue-link" data-issue-key="JDK-8185924" href="https://bugs.openjdk.java.net/browse/JDK-8185924" id="key-val" rel="4937332" style="font-family: Arial, sans-serif; font-size: 14px; color: rgb(59, 115, 175); text-decoration: none;">JDK-8185924</a>. Since we have been testing the fix with other changes together, I'll integrate them together with both CRs.<div class=""><br class=""></div><blockquote type="cite" class=""><div class=""><br class="">- g1HeapVerifier.cpp:63: I think this change is obsolete since<br class="">is_obj_dead_cond() will always return false. (I.e. some leftover of<br class="">some internal discussion)<br class=""></div></blockquote><div><br class=""></div><div>You are right. I reverted the change.</div><br class=""><blockquote type="cite" class=""><div class=""><br class="">- g1HeapVerifier.cpp: there is a verbose flag passed around. Not sure<br class="">if it should be kept, as it seems to be some code that has been used<br class="">for debugging this feature, but can't be activated anyway without code<br class="">changes.<br class=""></div></blockquote><div><br class=""></div>Removed.</div><div><br class=""><blockquote type="cite" class=""><div class=""><br class="">- heapRegion.inline.hpp:120: please fix the comment of the assert.<br class="">Something like "Closed archive regions should not have references into<br class="">other regionsâ€</div></blockquote><div><br class=""></div>Done.</div><div><br class=""><blockquote type="cite" class=""><div class=""><br class="">- heapRegion.inline.hpp:125: I think the existing code of faking open<br class="">archive regions as all-live does not work as implemented. Consider the<br class="">case when a new object in there is made live, and references in there<br class="">set to refer to some object outside this region, and is the only<br class="">reference (and it has not been marked live yet): if there is a<br class="">remembered set entry to that, and it is about to be scanned.<br class=""><br class="">The current implementation of HeapRegion::is_obj_dead() will consider<br class="">it dead, so we will enter the code path at line 125. Block_size_using<br class="">bitmap() will jump over that object, but the return values of<br class="">is_obj_dead_with_size() method will indicate the caller to not iterate<br class="">over this object anyway, potentially missing that reference.<br class=""><br class="">HeapRegion::is_obj_dead() needs to return that the object is not dead<br class="">for open archive regions. I think for now the safest way is to add<br class="">!is_open_archive() to the condition calculated there. That will obviate<br class="">the need for that existing hack to the assert too.<br class=""><br class="">It may have some perf impact though - actually recently there has been<br class="">some effort to remove that is_archive() check from that code (the one<br class="">that is now the is_closed_archive() assert). I do not see an easy way<br class="">to fix this. :( (i.e. there is likely no perf impact vs. jdk9 so it's<br class="">not that bad)<br class=""><br class="">This suggestion also only works with the assumption laid out in the CR<br class="">that there is no way that a live object can not become dormant again,<br class="">and the objects in the open archive regions are always parsable (never<br class="">contain junk data).<br class=""></div></blockquote><div><br class=""></div><div>Thank you for the analysis. I changed HeapRegion::is_obj_dead() with added !is_open_archive() condition as you suggested. I’m glad to get rid of the is_open_archive() change from is_obj_dead_with_size() assert.</div><div><br class=""></div><div>Thinking more about the case you described above, when an object (A) in the open archive just becomes live, there would be no reference to any other non-archive region at the moment. The object A only contains references to the archive (open or closed) regions initially. Scanning has no issue at the moment. When a new object (B) is allocated in the java heap and the reference is set in A. B is considered live, scanning would update the A->B reference accordingly. Is that correct?</div><br class=""><blockquote type="cite" class=""><div class=""><br class="">- HeapRegionType::relabel_as_old(): the code in line 175/176 is<br class="">unreachable and should be removed.<br class=""></div></blockquote><div><br class=""></div>Agreed. Removed.</div><div><br class=""></div><div>I’ll send out an updated webrev later. Thank you so much for all the help and contributions!!</div><div><br class=""></div><div>Jiangli</div><div><br class=""><blockquote type="cite" class=""><div class=""><br class="">I did not look through runtime code too closely.<br class=""><br class="">Thanks,<br class=""> Thomas<br class=""><br class=""></div></blockquote></div><br class=""></div></body></html>