RFR (S) 8240556: Abort concurrent mark after effective eager reclamation of humongous objects

[Thomas Schatzl]

Hi Liang,

On 07.08.20 11:08, Thomas Schatzl wrote:
> Hi Liang,
> 
>    first, apologies for the late answer; jdk15 release came inbetween, 
> then vacation time...
> 
> On 03.07.20 10:35, Liang Mao wrote:
>> Hi Man and G1 team,
>>
>> Previously we were discussing about abort of concurrent 
>> mark(JDK-8240556). I
>> made a concurrent cleaning bitmap version instead of the original STW 
>> one.
>> But there are still some other problems in real world. Recently I 
>> revisited
>> the implementation and make more optimization and the result seems 
>> very good.
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8240556
>> Webrev:
>>
> <http://cr.openjdk.java.net/~ddong/liangmao/8240556/>http://cr.openjdk.java.net/~ddong/liangmao/8240556/webrev.00/ 
> 
>> (This may related to https://bugs.openjdk.java.net/browse/JDK-8247928)
> I looked at it a bit, but need to go over my notes again.
> 
> Potentially we might want to first refactor the concurrent mark setup 
> code in G1CollectedHeap a bit to simplify it. Basically moving it 
> together a bit, and clearing out a few assumptions in some of the code 
> that mean that concurrent cycle == full marking.
> 
> I think from a functionality pov it is good (I have a recollection that 
> there may be one or two things missing, but I may be wrong as it 
> completely reuses the concurrent mark cycle).
> 
> More in a different thread.

- the change adds a new flag for the type of abort. This is something I 
would like to avoid as this effectively adds more states to the marking.

That change also adds more communication between G1CollectedHeap and 
G1ConcurrentMark, i.e. instead of using the existing methods 
(pre/post_concurrent_mark) to tell G1ConcurrentMark what it should do 
during the following concurrent phase, it adds a new one unnecessarily 
which are freely used.

- in G1ConcurrentMark::concurrent_cycle_abort_by_initial_mark() 
G1ConcurrentMark sets G1CollectorState::in_initial_mark_gc to false. I 
do not think this works (or is at least surprising in the future) to do 
all setup for the concurrent start pause and later do NOT do teardown as 
if you were in that same pause kind.

This indicates a problem in the existing code where you can't (and the 
change does not) tell other components (e.g. g1policy) about the new 
kind of abort at the moment.

(Looking at earlier reviews, this is the issue Man pointed out in point (2))

- in G1ConcurrentMark::concurrent_cycle_abort_by_fullgc() the condition 
for returning immediately seems wrong:

1972   if (!cm_thread()->during_cycle() || _aborted_by_fullgc) {

that should probably be _aborted_by_concurrent_mark in the second term.

- I do not like the name "Concurrent Mark Abort" for this operation as 
it is different to an actual abort (due to full gc). Something like 
"Undo" would be much better.

(StefanJ also already mentioned this in an earlier initial review, as 
well as Man)

- since jdk11 the "initial mark" pause has been renamed to "concurrent 
start". I know that the last remains of that has only been recently 
cleaned up...

- in G1ConcurrentMarkThread::run_service() all of the various phases are 
now decorated with some of (!has_aborted()) or (!aborted_by_fullgc()) or 
(!aborted_by_initial_mark) :)

While the current code does not seem wrong, this is now really ugly (if 
it hasn't been before ;)).

There is some change out (JDK-8247928) to refactor this; sometimes it's 
better to step back a little and do some preparatory cleanup/refactoring 
changes first.

- the change does not address the issue with taking heap resizing into 
account Man also brought up.

- the test only tests the log message, not behavior.

An initial prototype implementation based on JDK-8247928 is at

http://cr.openjdk.java.net/~tschatzl/8240556/webrev/

Thanks,
   Thomas