RFR: bug: Timely Reducing Unused Committed Memory

[Rodrigo Bruno]

Hi Thomas,

I am just wrapping up the patch with the new changes.

Three questions:

1 - regarding "use Ticks/Tickspan to measure time"

Currently I was using CollectedHeap::last_full_collection to get the time
of the last
attemp to compact/shrink. Since we are now giving the opportunity to shrink
the
heap without a full GC, would it make sense to have a different counter
(last_compaction_attemp or something) that would be updated inside
resize_if_necessary_after_full_collection?

2 - what checks should we have inside should_idle_compaction (previous
should_gc)?
Right now we have:
) time since last compaction attempt,
b) committed memory above user-defined threshold,
c) over committed mem is above used-defined threshold, and
d) load average is above user-defined threshold.

3 - it can happen now that two Concurrent Cycles are triggered with no
collection in between.
This breakes debug build asserts because we never
call  G1ConcurrentMark::post_initial_mark.
We cannot call directly from within CM main loop because we are outside a
safepoint. Any ideas?
Should we setup the next phase (i.e. call post_initial_mark) upon the
Cleanup phase?

cheers,
rodrigo

2018-08-06 15:40 GMT+02:00 Thomas Schatzl <thomas.schatzl at oracle.com>:

> Hi,
>
> On Mon, 2018-08-06 at 10:59 +0200, Rodrigo Bruno wrote:
> > Hi Thomas,
> >
> > thank you for your reply. No problem, w.r.t reponse times. I can also
> > take a while to respond...
> >
> > Let me just summarize my ideas for the next version of the patch
> > taking into consideration your comments.
> > This way we will see if we are both on the same page.
> >
> > 1 - move the should_gc check from vmthread loop
> > into G1ConcurrentMarkThread. I will use sleep_before_next_cycle
> > to trigger periodic checks (tune frequency).
>
> Yes. You might want adapt names a bit.
>
> >
> > 2 - inside should_gc:
> >   a) move the frequency test to first place
> >   b) use Ticks/Tickspan to measure time
>
> There are two reasons should_gc() would return: one the signalling of
> that CGC_lock, the other the timeout and request to shrink the heap.
>
> > 3 - when should_gc returns true,
> > trigger G1CollectedHeap::resize_after_full_collection
> > from a new VM operation
>
> Yes. That should already work.
>
> >
> > 4 - provide a flag UseFullGCForIdleCompaction to allow a full GC
> > instead of just G1CollectedHeap::resize_after_full_collection.
>
> Yes.
>
> >
> > 5 - use "Idle Time Compaction" for GC cause
>
> Yes.
>
> >
> > 6 - create one JUnit test.
>
> Please use jtreg, not junit :)
>
> The test should verify that idle timeout starts the expected VM
> operation, and the various other settings impact the behavior as
> expected.
>
> Thanks,
>   Thomas
>
> >
> > This is what I understood from your email. If you agree, I will
> > produce a new version of the patch with the proposed changes.
> >
> > cheers,
> > rodrigo
> >
> >
> > 2018-07-26 13:32 GMT+02:00 Thomas Schatzl <thomas.schatzl at oracle.com>
> > :
> > > Hi,
> > >
> > >   at least one sentence needs further clarification.
> > >
> > > On Thu, 2018-07-26 at 13:25 +0200, Thomas Schatzl wrote:
> > > > Hi Rodrigo,
> > > >
> > > >   first, sorry for taking so long to respond in this thread.
> > > >
> > > [...]
> > > > On Tue, 2018-06-19 at 20:46 +0200, Rodrigo Bruno wrote:
> > > > > Hi all,
> > > > >
> > > > > here is the first version of our contribution for draft JEP-
> > > > > 8204089.
> > > > >
> > > >
> > > >   some comments:
> > > >
> > > [...]
> > > > - looking a bit at other implementations it might be worth to be
> > > able
> > > > to customize what is been done when idle is detected.
> > > >
> > > > In most cases it might be sufficient to just shrink the heap (in
> > > a
> > > > new VM operation, using the existing code in
> > > > G1CollectedHeap::resize_after_full_collection() that already uses
> > > > Min/MaxHeapFreeRatio). A System.gc() seems very intrusive and
> > > should
> > > > be optional imho after some thinking; making this optional does
> > > not
> > > > seem too much work.
> > > >
> > > > Consider applications with more than a few GB of heap, those will
> > > be
> > > > affected a lot (i.e. unresponsive for multiple seconds)
> > > >
> > > > A flag like UseFullGCForIdleCompaction(?) could be used here.
> > > >
> > > > - the change does not make the system.gc() use "Idle" (probably
> > > "Idle
> > > > Time Compaction") or similar as suggested in the JEP.
> > >
> > >   ... as GC cause...
> > >
> > > It is very important to make sure that this action is easily
> > > identifiable for users.
> > >
> > > Thanks,
> > >   Thomas
> > >
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180819/725828e4/attachment.htm>