RFR: 8240239: Replace ConcurrentGCPhaseManager

[sangheon.kim at oracle.com]

On 3/6/20 2:30 PM, Kim Barrett wrote:
>> 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.
OK

>
>
>> 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.
OK

>
>> 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".
OK

>
>> 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.
>
>
I think I understand the exception, but I was feeling 'Unexpected 
un-support blah blah' or more detail saying 'current GC supports BP but 
WhiteBox check returned un-support blah blah' kind of message seem 
better explanation. But at least double negative (first example) would 
make more confused.

I had a chat with Kim and now I agree with all of his comments.
Sorry for noisy comments.

Looks good to me as is.

Thanks,
Sangheon