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

[William Kemper]

On Thu, 20 Apr 2023 21:38:59 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> William Kemper has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Re-enable TestArrayCopyStress
>
> 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.

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

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