RFR: 8226416: MonitorUsedDeflationThreshold can cause repeated async deflation requests [v4]

[David Holmes]

On Tue, 12 Jan 2021 20:30:13 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Adding support for a diagnostic option called NoAsyncDeflationProgressMax
>> with a default value of 3. If we have three async monitor deflation cycles in a
>> row with zero monitors deflated, then we adjust the in_use_list_ceiling up.
>> 
>> I've locally built and tested this fix on my MBP13 using the
>> MonitorUsedDeflationThresholdTest.java test that I wrote when this issue
>> first came up in June of 2019. I will be including this fix in my next Mach5
>> Tier[1-3] testing batch.
>
> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update comments for monitors_used_above_threshold() policy to include info about no-progress async monitor deflation cycles and the NoAsyncDeflationProgressMax option.

Hi Dan,
Thanks for the additional explanatory comments. The fix looks good to me.

A couple of comments on the test.

Thanks,
David

src/hotspot/share/runtime/synchronizer.cpp line 1170:

> 1168:     float remainder = (100.0 - MonitorUsedDeflationThreshold) / 100.0;
> 1169:     size_t new_ceiling = ceiling + (ceiling * remainder) + 1;
> 1170:     size_t old_ceiling = ObjectSynchronizer::in_use_list_ceiling();

You could capture:

size_t old_ceiling = ceiling;

back after line 1157, rather than re-reading it.

test/hotspot/jtreg/runtime/Monitor/MonitorUsedDeflationThresholdTest.java line 64:

> 62:         }
> 63: 
> 64:         Object obj = new Object();

This is a thread-local object, which means the synchronization has no affect, so I think C2 could elide the following sync-block. Using a static field would make it less likely that this can happen.

test/hotspot/jtreg/runtime/Monitor/MonitorUsedDeflationThresholdTest.java line 118:

> 116:             if (too_many == null) {
> 117:                 throw new RuntimeException("Did not find too_many string in output.\n");
> 118:             }

You should call output_detail.reportDiagnosticSummary() before throwing the exception. That emulates what OutputAnalyzer.shouldContain would do. (or you could just use shouldContain).

test/hotspot/jtreg/runtime/Monitor/MonitorUsedDeflationThresholdTest.java line 124:

> 122:             return;
> 123:         }
> 124: 

Please add a comment e.g.
`// else we are the exec'd java subprocess, so run the actual test

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

Changes requested by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1993