RFR: 8240239: Replace ConcurrentGCPhaseManager

[Kim Barrett]

> On Mar 6, 2020, at 2:40 AM, sangheon.kim at oracle.com wrote:
> 
> Hi Kim,
> 
> On 2/28/20 1:48 PM, Kim Barrett wrote:
>> Please review this change which removes the ConcurrentGCPhaseManager
>> class and replaces it with ConcurrentGCBreakpoints.
>> 
>> This is joint work with Per Liden.
>> 
>> This change provides a client API, used by WhiteBox.  The usage model
>> for a client is
>> 
>> (1) Acquire control of concurrent collection cycles.
>> 
>> (2) Do work that must be performed while the collection cycle is in a
>> known state.
>> 
>> (3) Request the concurrent collector run to a named "breakpoint", or
>> run to completion, and then hold there, waiting for further commands.
>> 
>> (4) Optionally goto (2).
>> 
>> (5) Release control of concurrent collection cycles.
>> 
>> Tests have been updated to use the new WhiteBox API.
>> 
>> This change provides implementations of the new mechanism for G1 and
>> ZGC.  A Shenandoah implementation is being left to others, but we
>> don't see any obvious reason for it to be difficult.
>> 
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8240239
>> 
>> Webrev:
>> https://cr.openjdk.java.net/~kbarrett/8240239/open.03/
> Looks good in general.

Thanks.

> But I have several minor nits.
> 
> ------------------
> src/hotspot/share/gc/g1/g1ConcurrentMarkThread.cpp
>  215         // Pause Remark.
> - Pre-existing: this comment should be moved to before  line 221.

I don't think the comment should be moved. The intervening stuff is
all related to the remark pause, and in particular that it demarcates
the completion of concurrent marking.


> 221         CMRemark cl(_cm);
> 
>  216         ConcurrentGCBreakpoints::at("BEFORE MARKING COMPLETED");
>  217         log_info(gc, marking)("Concurrent Mark (%.3fs, %.3fs) %.3fms",
> - Do we need to add time spent by 'at'? If we need time spent on 'at', it would be better to separate the log.

I don't think it matters much.  Note that we're also including the
time spent waiting on MMU.  The delay caused by being stopped at a
breakpoint obviously affects timing, one way or another.

> src/hotspot/share/gc/shared/concurrentGCBreakpoints.hpp
> 118   static void at(const char* breakpoint);
> - Don't we need more explanatory name? Something like reached_at? To me 'at' make me feel like the function would return none-void type. But this is my preference, so okay as is.

I think simply "at" has the desired meaning; paraphrasing from a
dictionary "expressing arrival in a particular place or position".
"reached_at" would be redundant.  The meaning I think you are
referring to is from the idiom container.at(element_designator),
where "at" is short for something like "reference/value at designated
location".

> test/hotspot/jtreg/gc/TestConcurrentGCBreakpoints.java
>  138                 throw new RuntimeException("Expected support");
> - Better explanation please as it is a bit confusing (at least) to me. I feel affirmative sentence seems not good for the message here. Maybe because I compared the message with the other case.

Sorry, but I'm not understanding the issue? In this case we expected
the current collector to support concurrent GC breakpoints, but it
doesn't, so we report a test failure that we expected support. In the
other case we expected the current collector to not support
breakpoints, but found that it claims that it does, so we report a
test failure that we have unexpected support. Both of these indicate a
mismatch between the expectations of the test and the capabilities of
the collector.