RFR (XS): 8140689: Skip last young-only gc if nothing to do in the mixed gc phase

[Derek White]

FYI: I replied to the wrong RFR, so the following discussion applies to 
8140689.

Hi Thomas,

TL;DR; (r)eviewed! But I still have some questions!

I'll top post here because I think I may misunderstand how things work.

I thought the current situation was something like this:
  1) Concurrent Marking has started.
  2) There are (potentially) a series of young-only GC happening 
concurrently.
  3) Concurrent Marking finishes. We call set_last_young_gc(true).
  4) A young-only GC finishes. Does concurrent marking happen during 
young-only GC, or is it suspended?
     - I have been assuming that 
record_concurrent_mark_cleanup_completed() might be called during the 
middle of a young-only GC.
  5) As the young-only GC finishes in record_collection_pause_end(), we 
realize that marking is finished and so if we need to we call 
set_gcs_are_young(false) (meaning now in mixed-mode).

But your comments below imply that a young-only gc has to start and 
finish after step 3 before we can move to mixed mode collection.

I think the problem might be my assumption that a mixed-mode GC is 
conceptually a GC with a slightly larger cset than a young-only GC. But 
maybe the code is treating them as separate phases - a young-only GC 
optionally followed by a partial old-gc. Even though the phrase "mixed" 
implies that it encompasses at least two parts :-)

Am I on the right track?

Some more comments below...

On 10/30/15 6:48 AM, Thomas Schatzl wrote:
> Hi Derek,
>
> On Thu, 2015-10-29 at 18:31 -0400, Derek White wrote:
>> Hi Thomas,
>>
>> I've been thinking of "last_young_gc" as really "consider doing a mixed
>> GC next time". So this gets set at the end of marking in
> I would already consider the last young-only gc as part of the mixed
> gc/reclamation phase. G1 only needs this gc to keep MMU. The problem is,
> that if we count that gc directly as mixed, current abort conditions of
> the mixed gc phase will be even more screwed up as they are now.
>
> We will look at these conditions in the future, but for now I would like
> to avoid messing with that.
OK.
>> record_concurrent_mark_cleanup_completed(), and checked in
>> record_collection_pause_end().
>>
>> So my question is why add a pre-filter in
>> record_concurrent_mark_cleanup_completed() when I think the same
>> condition is filtered out in record_collection_pause_end() any way?
> Note that I am not sure you replied to the correct RFR mail here. I
> think that question would fit more to the review for 8140689.
YES. Sorry for the confusion.
> Record_concurrent_mark_cleanup_completed is called before the last young
> gc is started, so we can avoid it completely. We want that, because in
> the RFR for 8140597 I made sure that the last young-only gc is not an
> initial mark gc. Doing so reduces the number of states (now really only
> "real" young-only gc can have an initial mark attached to it) which
> makes the code simpler, and also reinforces the idea that the last
> young-only gc is actually part of the mixed gc phase.
>
>> Another way to think of it is to scrap the notion of "last_young_gc"
>> completely, because that may or may not be true based on the READER of
>> that variable, not the SETTER. i.e. - You don't know if the current
>> young_only GC is the last one until it is over, and then you immediately
> That's not true. We already know before the start of the GC whether it
> will be the last young-only gc or not.

Really? Because in record_collection_pause_end() we might decide not to 
do any mixed-mode GC, so the next GC will also be young-only, so this 
young-only gc wasn't really last after all.
>> switch over to mixed_mode GC anyway, so I think it's more of a condition
>> than a state.
> I agree that we should try to scrap the last young-only gc by replacing
> it with a proper mixed gc (that eventually does not reclaim any old gen
> region). This however needs some consideration about the termination
> conditions of the mixed gc phase.
>
> I would prefer if we could wait with these changes after properly
> state'ifying the gc state. I.e. adding a state transition or removing
> one does not involve looking very hard through a lot of (unrelated)
> code.
>
>> Instead, rename "last_young_gc" to "concurrent_mark_completed". Then
>> unconditionally and unambiguously call
>> set_concurrent_mark_completed(true) in
>> record_concurrent_mark_cleanup_completed(), and check
> We do not want to unconditionally call
> set_concurrent_mark_completed(true) in
> record_concurrent_mark_cleanup_completed(), see RFR 8140689 :)
>
> In record_concurrent_mark_cleanup_completed(), if we already know that
> the following mixed gc phase will reclaim nothing, do not start it. This
> allows use to start another initial mark right after this one, making
> sure that the marking cycle can start and complete asap in this
> situation.
>
> Otherwise you will always have the intermediate last young-only gc which
> may fill up the old gen to a point where G1 cannot recover without a
> full gc. Which is the point of all these "get-to-initial-mark-asap"
> changes.
OK, I see the point to filtering out last_young_gc in 
record_concurrent_mark_cleanup_completed().
> I would at most rename it to something like "pre_mixed" or so :)
I've been getting hung up on the name "last_young_gc". The phrase "last" 
is not completely accurate, and isn't getting at what makes this young 
collection special. If we can nail that difference down then the naming 
should be simple.

  - Derek
>> concurrent_mark_completed() in record_collection_pause_end().
>>
>> This is a step in the direction of creating a state machine for the
>> concurrent mark phase. Does this make any sense?
>>
>>    - Derek
> Thanks,
>    Thomas
>
>

On 10/29/15 12:47 PM, Thomas Schatzl wrote:
> Hi all,
>
>    can I have reviews for another change that minimizes the turnaround
> time between one marking round and another one?
>
> In this case, if GC cleanup detects that there is nothing to reclaim, it
> will immediately request an initial mark for the next young-only gc.
> Previously there had been the requirement that the next young-only gc
> after marking must be a "regular" young-only gc.
> This wastes some time until we have a new up-to-date view of the
> liveness, potentially ending in full gcs.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8140689
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8140689/webrev
> Testing:
> jprt, vm.gc, manual testing
>
> Thanks,
>    Thomas
>
>