RFR: 8240556: Abort concurrent mark after effective eager reclamation of humongous objects [v2]

[Thomas Schatzl]

On Tue, 22 Sep 2020 08:45:35 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:

>> Thomas Schatzl has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev
>> excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since
>> the last revision:
>>  - reviews sjohanns and ayang; merged Command and Status plus cleanup
>>  - Remove test that only accidentally worked: G1 never sent the AllocationRequiringGC
>>    event for GCs caused by going over the IHOP threshold for humongous allocations.
>>    
>>    That test previously only worked because G1 could not reclaim the humongous objects
>>    fast enough so crossing the IHOP threshold causes a full concurrent mark. Allocations
>>    during that concurrent mark do not cause a GC that can reclaim these objects, so
>>    ultimately some young GC that sends the AllocationRequiringGC event will be sent.
>>    
>>    With concurrent undo this is not guaranteed any more, i.e. only in environments where
>>    concurrent undo is slow (and we'll improve it soon) this test works.
>>    
>>    The test is too timing dependent, so removing it.
>>  - Fixed test
>>  - Initial import
>
>>   I dislike putting this pause-related state into global state. Unless you or somebody else insists on this I would like
>>   to keep the code.
>> 
>> Filed JDK-8253343 which among other things should clean this up and make it possible to easily manage pause-only state.
> 
> I get your argument about not putting this into the collector-state and I'm fine with leaving it like this for now. I
> think this update really helped and I have two more comments on how to reduce the "state passing" even more. Let me
> know what you think.

Testing of the latest changes, i.e. 51e0ad9: tier1-3

> src/hotspot/share/gc/g1/g1Policy.cpp line 790:
> 
>> 788:     if (!concurrent_operation_is_full_mark) {
>> 789:       abort_time_to_mixed_tracking();
>> 790:     }
> 
> More or less same idea here. What do you think about moving this to a separate function, that is either called from
> `start_concurrent_cycle()` or `concurrent_undo_cycle_do()`?

The reason for this abort_time_to_mixed_tracking() call is that we need to undo what we've done in record_pause()
earlier in the same method. That is why the change does the undo of that part here.

Also, this would place policy state changes outside of the application of the gc algorithm to the heap (I consider
changes that are between the gc_prologue() and gc_epilogue() "young collection algorithm"). Putting this would break
that assumption.

I fixed this in the most recent change by making the concurrent start pause that starts an undo explicit so that
record_pause() does not start .

-------------

PR: https://git.openjdk.java.net/jdk/pull/177