<div dir="ltr">Hi Thomas,<div><br></div><div>I am just wrapping up the patch with the new changes.</div><div><br></div><div>Three questions:</div><div><br></div><div>1 - regarding "<span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">use Ticks/Tickspan to measure time"</span><br style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"></div><div><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br></span></div><div><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">Currently I was using CollectedHeap::last_full_colle<wbr>ction to get the time of the last</span></div><div><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">attemp to compact/shrink. Since we are now giving the </span><span style="font-size:12.8px">opportunity to shrink the </span></div><div><span style="font-size:12.8px">heap without a full GC, would it make sense to have a different </span><span style="font-size:12.8px">counter </span></div><div><span style="font-size:12.8px">(last_compaction_attemp or something) that would be updated inside </span></div><div><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">resize_if_necessary_after_full<wbr>_collection?</span></div><div><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br></span></div><div><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">2 - what checks should we have inside should_idle_compaction (previous should_gc)?</span></div><div><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">Right now we have:</span></div><div><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">) time since last compaction attempt, </span></div><div><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">b) committed memory above user-defined threshold,</span></div><div><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">c) over committed mem is above used-defined threshold, and </span></div><div><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">d) load average is above user-defined threshold.</span></div><div><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br></span></div><div><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">3 - it can happen now that two Concurrent Cycles are triggered with no collection in between.</span></div><div><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">This breakes debug build asserts because we never call  G1ConcurrentMark::post_initial_mark.</span></div><div><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">We cannot call </span><span style="font-size:12.8px">directly from within CM main loop because we are outside a safepoint. Any ideas?</span></div><div><span style="font-size:12.8px">Should </span><span style="font-size:12.8px">we setup the </span><span style="font-size:12.8px">next phase (i.e. call post_initial_mark) upon the Cleanup phase?</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">cheers,</span></div><div><span style="font-size:12.8px">rodrigo</span></div><div class="gmail_extra"><br><div class="gmail_quote">2018-08-06 15:40 GMT+02:00 Thomas Schatzl <span dir="ltr"><<a href="mailto:thomas.schatzl@oracle.com" target="_blank">thomas.schatzl@oracle.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
On Mon, 2018-08-06 at 10:59 +0200, Rodrigo Bruno wrote:<br>
> Hi Thomas,<br>
> <br>
> thank you for your reply. No problem, w.r.t reponse times. I can also<br>
> take a while to respond...<br>
> <br>
> Let me just summarize my ideas for the next version of the patch<br>
> taking into consideration your comments.<br>
> This way we will see if we are both on the same page.<br>
> <br>
> 1 - move the should_gc check from vmthread loop<br>
> into G1ConcurrentMarkThread. I will use sleep_before_next_cycle<br>
> to trigger periodic checks (tune frequency).<br>
<br>
Yes. You might want adapt names a bit.<br>
<br>
> <br>
> 2 - inside should_gc:<br>
>   a) move the frequency test to first place<br>
>   b) use Ticks/Tickspan to measure time<br>
<br>
There are two reasons should_gc() would return: one the signalling of<br>
that CGC_lock, the other the timeout and request to shrink the heap.<br>
<br>
> 3 - when should_gc returns true,<br>
> trigger G1CollectedHeap::resize_after_<wbr>full_collection<br>
> from a new VM operation<br>
<br>
Yes. That should already work.<br>
<br>
> <br>
> 4 - provide a flag UseFullGCForIdleCompaction to allow a full GC<br>
> instead of just G1CollectedHeap::resize_after_<wbr>full_collection.<br>
<br>
Yes.<br>
<br>
> <br>
> 5 - use "Idle Time Compaction" for GC cause<br>
<br>
Yes.<br>
<br>
> <br>
> 6 - create one JUnit test.<br>
<br>
Please use jtreg, not junit :)<br>
<br>
The test should verify that idle timeout starts the expected VM<br>
operation, and the various other settings impact the behavior as<br>
expected.<br>
<br>
Thanks,<br>
  Thomas<br>
<br>
> <br>
> This is what I understood from your email. If you agree, I will<br>
> produce a new version of the patch with the proposed changes.<br>
> <br>
> cheers,<br>
> rodrigo<br>
> <br>
> <br>
> 2018-07-26 13:32 GMT+02:00 Thomas Schatzl <<a href="mailto:thomas.schatzl@oracle.com">thomas.schatzl@oracle.com</a>><br>
> :<br>
> > Hi,<br>
> > <br>
> >   at least one sentence needs further clarification.<br>
> > <br>
> > On Thu, 2018-07-26 at 13:25 +0200, Thomas Schatzl wrote:<br>
> > > Hi Rodrigo,<br>
> > > <br>
> > >   first, sorry for taking so long to respond in this thread.<br>
> > > <br>
> > [...]<br>
> > > On Tue, 2018-06-19 at 20:46 +0200, Rodrigo Bruno wrote:<br>
> > > > Hi all,<br>
> > > > <br>
> > > > here is the first version of our contribution for draft JEP-<br>
> > > > 8204089. <br>
> > > > <br>
> > > <br>
> > >   some comments:<br>
> > > <br>
> > [...]<br>
> > > - looking a bit at other implementations it might be worth to be<br>
> > able<br>
> > > to customize what is been done when idle is detected.<br>
> > > <br>
> > > In most cases it might be sufficient to just shrink the heap (in<br>
> > a<br>
> > > new VM operation, using the existing code in<br>
> > > G1CollectedHeap::resize_after_<wbr>full_collection() that already uses<br>
> > > Min/MaxHeapFreeRatio). A System.gc() seems very intrusive and<br>
> > should<br>
> > > be optional imho after some thinking; making this optional does<br>
> > not<br>
> > > seem too much work.<br>
> > > <br>
> > > Consider applications with more than a few GB of heap, those will<br>
> > be<br>
> > > affected a lot (i.e. unresponsive for multiple seconds)<br>
> > > <br>
> > > A flag like UseFullGCForIdleCompaction(?) could be used here.<br>
> > > <br>
> > > - the change does not make the system.gc() use "Idle" (probably<br>
> > "Idle<br>
> > > Time Compaction") or similar as suggested in the JEP.<br>
> > <br>
> >   ... as GC cause...<br>
> > <br>
> > It is very important to make sure that this action is easily<br>
> > identifiable for users.<br>
> > <br>
> > Thanks,<br>
> >   Thomas<br>
> > <br>
> <br>
> <br>
<br>
</blockquote></div><br></div></div>