JDK-8256307 : cleanup AvgMonitorsPerThreadEstimate and _in_use_list_ceiling types
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: 16
  • Priority: P4
  • Status: Resolved
  • Resolution: Duplicate
  • OS: generic
  • CPU: generic
  • Submitted: 2020-11-12
  • Updated: 2021-01-13
  • Resolved: 2021-01-13
Related Reports
Duplicate :  
Relates :  
Relates :  
Description
[~dholmes] raised this issue during the code review for JDK-8253064:

 src/hotspot/share/runtime/synchronizer.cpp
// ObjectMonitors than the estimated average.
//
// Start the ceiling with the estimate for one thread:
jint _in_use_list_ceiling = AvgMonitorsPerThreadEstimate;

@dholmes-ora
Why is this a jint when you use size_t for its accessor and all the other sizes that you compare with the ceiling are also size_t?
I'm not sure size_t is right to use in these cases (do we really expect different maximums on 32-bit versus 64-bit?) but it should be all or none IMO.

@coleenp
Our int types are really confused. AvgMonitorsPerThreadEstimate is defined as an intx which is intptr_t and the range of it is 0..max_jint which is 0 .. 0x7fffffff . jint is long on windows (the problematic type) and int on unix. Since this is a new declaration, it probably should be something other than jint but what?
At any rate, it should be declared as 'static'.

@dcubed-ojdk 
_in_use_list_ceiling is a jint because we've specified the range
as 0..max_jint and I wanted some sanity to that variable's type.

If I change _in_use_list_ceiling to size_t, then I get a compile time
error probably because AvgMonitorsPerThreadEstimate is an intx
which (I think) is my only choice for a command line option.

@fisk will have to chime in with the background on why he picked size_t.

@dcubed-ojdk
@coleenp - Nice catch on the missing 'static'.

@fisk
I typically use size_t for entities that can scale with the size of the machine's memory, so I don't have to think about whether there are enough bits. Could AvgMonitorsPerThreadEstimate be uintx instead of intx? And then maybe we don't need to declare a range, as the natural range of the uintx seems perfectly valid.
Comments
Thanks [~dholmes]! I'm closing this bug as a duplicate of JDK-8259349.
13-01-2021

Changing to size_t addresses my "all or nothing" concern. I dislike size_t here because (to use Erik's words) these numbers do not need to scale with the size of memory. Whenever I see size_t I wonder why a field needs to be 32-bit or 64-bit. Bottom line: JDK-8259349 has addressed this issue IMO.
13-01-2021

I believe my changes in JDK-8259349 will be addressing this bug... I'll check with [~dholmes] and make sure that he is satisfied once we're done with JDK-8259349.
11-01-2021

[~dholmes] - feel free to update the RFE's description as needed.
12-11-2020