RFR (M/L): 8247928: Refactor G1ConcurrentMarkThread for mark abort (JDK-8240556)

[Thomas Schatzl]

Hi Kim,

On 03.09.20 09:24, Kim Barrett wrote:
>> On Aug 21, 2020, at 9:44 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>> http://cr.openjdk.java.net/~tschatzl/8247928/webrev.0_to_1/ (diff)
>> http://cr.openjdk.java.net/~tschatzl/8247928/webrev.1/ (full)
> 
> I started to review this, but I’m confused.  The baseline for webrev.1 doesn’t look like what’s
> in the repository.  And I don’t see some other RFR that this is based on.  It looks like no other
> version that I’ve seen.

   sorry, my screwup when specifying the correct revision to create the 
webrev from - it's based on the state machine prototype posted earlier 
in the thread.

> 
> One thing I noticed despite that:
> 
> It seems backward that the phase functions return true to indicate aborted,
> overflow, or whatever, and return false for successful completion. And
> there's no mention of what the return values are in the description in the
> header file.
> 

I think the code is easier to read without the need to always negate the 
result. Ie. in the following example

if (!phase1) return;
if (!phase2) return;

the "!" detracts imho from reading the steps.

Another option I can see could be a macro hiding all this, e.g.

EXEC(phase1);
EXEC(phase2);

but I kind of dislike that even more.

Added documentation.

http://cr.openjdk.java.net/~tschatzl/8247928/webrev.1_to_2 (diff)
http://cr.openjdk.java.net/~tschatzl/8247928/webrev.2 (full)

If you think that either the explicit negation or some macro is much 
better or you have another, better idea, I can change it.

Thanks,
   Thomas