RFR: 8240239: Replace ConcurrentGCPhaseManager

[sangheon.kim at oracle.com]

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

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

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

------------------
For the record.
I asked Kim about better alternative for 'const char*' at 
ConcurrentGCBreakpoints::run_to(const char* breakpoint) and 
ConcurrentGCBreakpoints::at(const char* breakpoint) something like 
static member or enum type. The reason is that such string will locate 
several places and there is already static member in WhiteBox.java.
However, the breakpoint may vary among collectors and it is open set. 
And currently there are only 2 breakpoints, so Kim(and maybe Per) 
decided just not think hard about it.
I am fine with it too.

Thanks,
Sangheon


>
> To possibly simplify the review, the open patch is also provided as a
> pair of patches, one for removing the old mechanism and a second to
> add the new mechanism.
>
> https://cr.openjdk.java.net/~kbarrett/8240239/remove_phase_control.03/
> Removes ConcurrentGCPhaseManager and its G1 implementation, except
> that tests are not modifed.
>
> https://cr.openjdk.java.net/~kbarrett/8240239/control.03/
> Adds ConcurrenGCBreakpoints, with G1 and ZGC implementations, and
> updates tests to use it.
>
> Testing:
> mach5 tier1-5, which includes all the updated and new tests.
>