RFR: 8259349: -XX:AvgMonitorsPerThreadEstimate=1 does not work right [v4]

[David Holmes]

On 9/01/2021 8:50 am, Daniel D.Daugherty wrote:
> On Fri, 8 Jan 2021 22:30:15 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>>> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
>>>
>>>    delete bad assert found by linux build.
>>
>> src/hotspot/share/runtime/globals.hpp line 718:
>>
>>> 716:           range(0, max_jint)                                                \
>>> 717:                                                                             \
>>> 718:   /* notice: the max range value here is max_jint, not max_intx  */         \
>>
>> The comment is no longer applicable.
> 
> Deleted
> 
>> src/hotspot/share/runtime/synchronizer.cpp line 257:
>>
>>> 255: // which is called after cmd line options are processed.
>>> 256: // This is a 'jint' because the range of AvgMonitorsPerThreadEstimate
>>> 257: // is 0..max_jint:
>>
>> Do we need this comment now?
> 
> Yes. The range for AvgMonitorsPerThreadEstimate is still 0..max_jint.

But the "This is jint" part is no longer relevant, the comment was 
really reading:

This is a jint [rather than intx] because the range of AMPTE is 
0..max_jint [not 0..max_intx]

but now AMPTE is an int we don't need to make that clarification on the 
type.

>> src/hotspot/share/runtime/synchronizer.cpp line 1178:
>>
>>> 1176: }
>>> 1177:
>>> 1178: void ObjectSynchronizer::set_in_use_list_ceiling(size_t new_value) {
>>
>> new_value should just be declared as int or jint now and the cast and comments removed.
> 
> I don't think so. For some reason @fisk used size_t for MonitorList
> _count and _max fields so the ceiling that is passed in and returned
> should be size_t. The only reason that the _in_use_list_ceiling is jint
> is because the range is 0..max_jint. At one point I tried to change it
> to size_t and ran into build issues, but I don't remember the details.

If you have a jint field being set by a setter function then the setter 
function should take a jint argument. The types of the other fields is 
not really relevant. But this mish-mash of int/jint/size_t really bugs 
me. :(

Cheers,
David

> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/1992
>