<div dir="ltr"><div dir="ltr">Hi Man,<div><br></div><div>As both Thomas and Per seemed to be okay with the refactoring, you probably had sufficient approval already. </div><div><br></div><div>The following comment in adaptiveSizePolicy.cpp seems to be outdated. The comment predates the PermGen removal. It's probably a good idea to also cleanup the comment as part of your refactoring change.</div><div><span style="color:blue"><br></span></div><div><pre style="color:rgb(0,0,0)"><span class="gmail-changed" style="color:blue"> 281 // Note that the gc time limit test only works for the collections</span>
<span class="gmail-changed" style="color:blue"> 282 // of the young gen + tenured gen and not for collections of the</span>
<span class="gmail-changed" style="color:blue"> 283 // permanent gen. That is because the calculation of the space</span>
<span class="gmail-changed" style="color:blue"> 284 // freed by the collection is the free space in the young gen +</span>
<span class="gmail-changed" style="color:blue"> 285 // tenured gen.</span></pre><pre style="color:rgb(0,0,0)">Thanks and regards,</pre><pre style="color:rgb(0,0,0)">Jiangli</pre><pre style="color:rgb(0,0,0)"><br></pre></div></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Mar 1, 2019 at 1:43 PM Man Cao <<a href="mailto:manc@google.com">manc@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div>Hi,</div><div><br></div><div>I rebased the patch to latest tip:</div><div><a href="https://cr.openjdk.java.net/~manc/8212206/webrev.03/" target="_blank">https://cr.openjdk.java.net/~manc/8212206/webrev.03/</a><br></div><div dir="ltr"><br></div><div>Could someone give a second "looks good"?</div><br clear="all"><div><div dir="ltr" class="gmail-m_8381197957835542938gmail_signature"><div dir="ltr">-Man</div></div></div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Jan 26, 2019 at 4:35 PM Man Cao <<a href="mailto:manc@google.com" target="_blank">manc@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr">Friendly ping. Could anyone give a second "looks good"?<div><br></div>As for the develop flag AdaptiveSizePolicyGCTimeLimitThreshold/GCOverheadLimitThreshold, I added a note about it in <a href="https://bugs.openjdk.java.net/browse/JDK-8212084" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8212084</a>.<div><div><br clear="all"><div><div dir="ltr" class="gmail-m_8381197957835542938gmail-m_7865355997339998338gmail_signature"><div dir="ltr">-Man</div></div></div><br></div></div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jan 15, 2019 at 6:41 PM Man Cao <<a href="mailto:manc@google.com" target="_blank">manc@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div>Hi,</div><div><br></div><div>I rebased the patch to tip and updated year in some headers to 2019, without making any real change:</div><div><a href="http://cr.openjdk.java.net/~manc/8212206/webrev.02/" target="_blank">http://cr.openjdk.java.net/~manc/8212206/webrev.02/</a></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I don't foresee that this will be implemented, or even makes sense, for <br>ZGC. As I see it, this is only a thing STW collectors. For that reason, <br>I don't think it belongs in CollectedHeap. Keeping it as a separate <br>utility class for collectors that want to use it sounds better.<br></blockquote><div>Sounds good to keep this patch in the current state, without further changing the CollectedHeap class.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I haven't looked very closely at the patch, but couldn't help to notice <br>that the option is called "GCOverheapLimitThreshold" (and <br>"AdaptiveSizePolicyGCTimeLimitThreshold" before that), which is a <br>tautology and a not very good description of what it is.<br>How about we take the opportunity to clean this up and completely ditch <br>the "gc_overhead_limit_count" thing and get rid of this option? It's a <br>"develop" option, so it's not available to normal users anyway. Has <br>anyone of you ever used this option and actually find it valuable?</blockquote><div>I didn't find any users inside Google that require changing this option.</div>That said, some users did complain that UseGCOverheadLimit for ParallelGC or CMS is too difficult to get<br>triggered, because of the requirement for 5 consecutive full GCs, which is set by this option.</div><div dir="ltr">I think if it were a normal "product" option, there will definitely be users setting it.</div><div dir="ltr">I never understand why it is a "develop" option. I think we could either remove it,</div><div dir="ltr">or make it an "experimental" option.</div><div dir="ltr">I'm leaning towards not removing it for now, as I'm not sure if 5 is still a reasonable<br></div><div>default value for UseGCOverheadLimit for G1.</div><div>How about we decide whether to keep or remove this option after</div><div>JDK-8212084 (UseGCOverheadLimit for G1) is fixed?</div><div><br></div><div dir="ltr"><div>Also for the hsperfdata counter change, I created <a href="https://bugs.openjdk.java.net/browse/JDK-8217221" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8217221</a>. I will draft a CSR for it later.</div><div><br></div><div dir="ltr"><div><div dir="ltr" class="gmail-m_8381197957835542938gmail-m_7865355997339998338gmail-m_-1398394828689451322gmail-m_3466406192080524826m_-2867842813224295061gmail_signature"><div dir="ltr">-Man</div></div></div></div></div></div></div></div>
</blockquote></div>
</blockquote></div>
</blockquote></div></div>