RFR (S): 8138740: Start initial mark right after mixed GC if needed

[Derek White]

Hi Thomas,

You are right, I replied to the wrong RFR! Most of my comments don't 
make sense.

8138740 looks fine to me.

  - Derek

ps. I'll copy this discussion over to the correct RFR.

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.
>
>> 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.
>
> 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.
>
>> 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.
>
> I would at most rename it to something like "pre_mixed" or so :)
>
>> 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
>
>