RFR: 8333105: Shenandoah: Results of concurrent mark may be lost for degenerated cycle

[Y. Srinivas Ramakrishna]

On Wed, 29 May 2024 19:29:51 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> If Shenandoah does not detect a cancellation after completing concurrent mark, but before completing final mark it will fall through the concurrent cycle without collecting anything. This will shortly lead to a degenerated cycle which will _not_ use the results from the nearly complete concurrent mark. This will result in an unnecessarily longer degenerated cycle.
>
> src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp line 147:
> 
>> 145:     bool cancelled = check_cancellation_and_abort(ShenandoahDegenPoint::_degenerated_mark);
>> 146:     assert(cancelled, "GC must have been cancelled between concurrent and final mark");
>> 147:     return false;
> 
> Not your change, but I notice that the semantics of the return value hasn't been explicitly documented anywhere in the class hierarchy. Might make sense to place a line of documentation, say in `ShenandoahGC` which first declares the virtual method, even something simple such as:
> 
> 
> // Returns true if the collection was completed successfully, false otherwise.

It also occurs to me that the pattern here is, roughly of the following form:


  do_some_uninterruptible_operation();
  check cancellation status & return if cancelled, else continue to next uninterruptible operation;
  ...


In that case, the checks should not be on the state of the progress of the collection, but on whether the cycle has been cancelled, and the asserts should then check the state of the collection; in other words, the dual of the structure of control in this method.

That said, I do not understand if there's a deeper rationale here in that sometimes we will want to skip a next uninterruptible phase of the collection regardless, because the state machine of the collection explicitly transitions us, under appropriate conditions, into a different phase than the otherwise normal flow, independent of whether a cancellation happened.

For example, what would be the difference between your current test:


  if (concurrent_mark_in_progress()) {


vs, say:


  if (!evacuation_in_progress()) {


vs : 


   if (check_cancellation()) {
     ... return false;
   }
   assert(!concurrent_mark_in_progress(), "Should have completed");
   assert(evacuation_in_progress(), "Next logical phase of collection");


Just wondering if a more uniform idiom might make for easier code patterns and easier, more uniform, reasoning about correctness.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19434#discussion_r1619395782