RFR: 8306334: Handle preemption of old cycle between filling and bootstrap phases [v2]

[Y. Srinivas Ramakrishna]

On Thu, 20 Apr 2023 22:55:00 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 541:
>> 
>>> 539:       // have been preempted before we start a bootstrap cycle.
>>> 540:       if (check_cancellation_or_degen(ShenandoahGC::_degenerated_outside_cycle)) {
>>> 541:         if (heap->cancelled_gc()) {
>> 
>> In most of the other states where cancellation is checked, we only check `heap->cancelled_gc()`, but here we first `check_cancellation_or_degen()` before checking `cancelled_gc()`.
>> 
>> Now that method is below, where I have annotated some comments in between lines of code (github doesn't allow me to comment on lines not touched in a  PR).
>> 
>> 
>> bool ShenandoahControlThread::check_cancellation_or_degen(ShenandoahGC::ShenandoahDegenPoint point) {
>>   ShenandoahHeap* heap = ShenandoahHeap::heap();
>>   if (!heap->cancelled_gc()) {
>>     return false;
>>   }
>> 
>> 
>> In other words, if `check_cancellation_..` answers true, it must be be that `cancelled_gc()` is true already so the test at line 541 ~~is~~ seemed redundant at first blush. (Which was corrected upon reading further below.)
>> 
>> However, there are more circumstances in which `check_cancellation_..` will answer true below.
>> 
>> 
>> 
>>   if (in_graceful_shutdown()) {
>>     return true;
>>   }
>> 
>>   assert(_degen_point == ShenandoahGC::_degenerated_outside_cycle,
>>          "Should not be set yet: %s", ShenandoahGC::degen_point_to_string(_degen_point));
>> 
>>   if (is_alloc_failure_gc()) {
>>     _degen_point = point;
>>     return true;
>>   }
>> 
>> The assignment of `_degen_point = point` seems to be equivalent to assigning `_degen_point = ShenandoahGC::_degenerated_outside_cycle`, because of the assertion further above. So the comment in the assertion and the assignment further below look a bit fishy to me; as if code changed and these code paths may nt have been updated? Anyway, it seemed a bit confusing; such an assignment also happens further down in this method (code below).
>> 
>> 
>> 
>>   if (_preemption_requested.is_set()) {
>>     assert(_requested_generation == YOUNG, "Only young GCs may preempt old.");
>>     _preemption_requested.unset();
>> 
>>     // Old generation marking is only cancellable during concurrent marking.
>>     // Once final mark is complete, the code does not check again for cancellation.
>>     // If old generation was cancelled for an allocation failure, we wouldn't
>>     // make it to this case. The calling code is responsible for forcing a
>>     // cancellation due to allocation failure into a degenerated cycle.
>>     _degen_point = point;
>>     heap->clear_cancelled_gc(false /* clear oom handler */);
>>     return true;
>>   }
>> 
>> I see that above we clear the cancellation, so I can see the case where `check_cancellation_` would answer true, but a subsequent `cancelled_gc` would not. So I see the case where we are doing the extra subsequent check after returning from this method. Still, this seems a bit subtle and I wonder if these could not be somehow made less subtle (and possibly cause confusion or maintenance issues). Anyway, this would be for some future clean-up.
>> 
>> 
>>   fatal("Cancel GC either for alloc failure GC, or gracefully exiting, or to pause old generation marking");
>>   return false;
>> }
>> 
>> 
>> Anyway, I thought I'd point this out, although I don't understand all of the various interactions here well enough to do a real review.
>
> Yes, the name `check_cancellation_or_degen` does not suggest at the complexity or side effects of the method.
> 
>> The assignment of _degen_point = point seems to be equivalent to assigning _degen_point = ShenandoahGC::_degenerated_outside_cycle, because of the assertion further above.
> 
> The assertion is checking the value of `_degen_point` _before_ the assignment, asserting that we do not overwrite a `_degen_point` that has already been set.

Of course, I don't know what I was thinking. Thanks!

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/262#discussion_r1174160415