RFR: 8305994: Guarantee eventual async monitor deflation [v2]

[Volker Simonis]

On Fri, 14 Apr 2023 18:07:27 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Ok, glad we agree on that one now :)
>> 
>> However, I just realized that with the explicit flag you now do not reset `_no_progress_cnt` back to zero in the case where you do deflate monitors in a round triggered by `GuaranteedAsyncDeflationInterval`. Wouldn't it be better to do that?
>
> No, I don't think so, mostly because this patch tries to build something that: a) works reliably; b) does not break other stuff accidentally. Let's isolate the triggers instead of trying to make sure they work together well.
> 
> Consider a case where there is a sequence of several no-progress threshold cleanups, this would normally collect enough `_no_progress_cnt`-s to bump the threshold ceiling in order to break the sequence. Note that no-progress means we tried to deflate, but deflated none. So if there is a guaranteed cleanup in the middle of that sequence which drops `_no_progress_cnt = 0` and still does not deflate anything (which is likely), the threshold cleanup code would start another series of cleanups from the beginning. And this is the simplest interaction example I can think of...

Sorry for being so impertinent :) but I think "*if there is a guaranteed cleanup in the middle of that sequence which drops `_no_progress_cnt` = 0 and still does not deflate anything*" can not happen. `_no_progress_cnt` will only be dropped to zero if there was progress (i.e. if we really did deflate).

I think the problem is exactly the other way round. Imagine we do a sequence of several no-progress threshold cleanups, which collect some `_no_progress_cnt`-s but not enough to bump the threshold ceiling in order to break the sequence because eventually one of the threshold cleanups deflates some monitors and resets `_no_progress_cnt` to zero. Now with your change this cleanup which deflated some monitors might happen during a guaranteed cleanup and because of your new flag it will not reset `_no_progress_cnt` as before and a few follow up no-progress threshold cleanups will then unnecessarily bump the threshold ceiling.

So I still think the `--_no_progress_cnt` would be the more "original behavior-preserving" solution, or if you want to go with the extra flag then probably something like:

    if (deflated_count != 0) {
      _no_progress_cnt = 0;
    } else {
      if (_no_progress_allow_updates) {
        _no_progress_cnt++;
      }
    }

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13474#discussion_r1167219372