RFR (S) 8235765: Use of the long type should be avoided in shared code

[David Holmes]

Hi Coleen,

And it seemed so simple when you started :)

On 13/08/2020 6:43 am, Coleen Phillimore wrote:
> 
> Hi Vladimir,  Thank you for looking at this change.
> 
> On 8/12/20 1:25 PM, Vladimir Kozlov wrote:
>> Hi Coleen,
>>
>> I think it is safe to use 'uint' (uint32_t) for all counts in sweeper.

"long" is only 32-bit on 64-bit Windows, so if we don't see any issues 
on Windows then there is a case to be made that all these long fields 
would appear to be fine if only 32-bit ... that said some of them 
"obviously" look like they should be size_t as you have made them.

>> What is a story about using int64_t vs jlong? And others *_t vs j* types.
> 
> jlong is a signed type (either long or long long) so in mutex, even  > though uint64_t makes more sense, I used int64_t so that they'd be
> convertible to jlong in the PlatformMutex layer.  I didn't want to pull 
> the string of this sweater even further to convert the jlong to uint64_t 
> in that layer. (If that's even the right thing to do).  We have been 
> trying to avoid using java types like jint, jlong etc, in shared code, 
> but they're pretty much everywhere.

We've been avoiding unnecessary use of j* types in the VM (shared or 
not) but when the values represent values to/from Java code then the j* 
types are appropriate. The multiple abstractions 
Parker/ParkEvent/PlatformEvent/PlatformParker/PlatformMonitor/Monitor 
make it hard to see exactly how values flow through, and which ones come 
direct from Java. We should keep the jlong at the Java-connected api 
level and use uint64_t elsewhere. Separate RFE of course. :)


Looking at webrev v02:

src/hotspot/share/memory/filemap.hpp

You changed the field from long to int64_t but you didn't change the 
accessor:

87   long   filesize()  const { return _filesize; }

that could give a truncation warning on Windows. That field is set from 
the stat st_size field, which is defined as type off_t on non-Windows 
and as ... okay I can't make sense of the win32 docs to figure out 
whether a plain stat will be a 64-bit or 32-bit [1]. So not clear what 
the right type is here - but the field and accessor should match.

[1] 
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/stat-functions?view=vs-2019

---

src/hotspot/share/utilities/elfFile.cpp

fseek is specified to take a long for the offset, so this change would 
be a problem on 32-bit builds I think.

Thanks,
David
-----



>>
>> Also you need to look on JFR code which collect these data:
>>
>> src/hotspot//share/jfr/periodic/jfrPeriodic.cpp: 
>> event.set_methodReclaimedCount(NMethodSweeper::total_nof_methods_reclaimed()); 
>>
>>
>> src/hotspot//share/jfr/metadata/metadata.xml:    <Field type="int" 
>> name="methodReclaimedCount" label="Methods Reclaimed" />
>>
>> And I found that metadata.xml have several 'long' uses too:
>>
>> src/hotspot//share/jfr/metadata/metadata.xml:    <Field type="long" 
>> contentType="millis" name="peakTimeSpent" label="Peak Time" />
>>
>> Looking on codecache code and sweeper and I see a lot of 
>> inconsistencies in used types :(
>> May be we need an other (compiler) RFE to clean that up.
> 
> Yes, I agree. I'm going to revert sweeper, nmethod and vmStructs. It's 
> better that 'long' is fixed individually in the sweeper and associated 
> files.
> 
> The JFR metadata.xml has a lot of "long" types declared in it.  I'm 
> going to revert compileBroker.* too.   This is going to have to be fixed 
> a little at a time.
> 
> I'm testing a new more limited version of this change now.
> 
> Thanks,
> Coleen
>>
>> Regards,
>> Vladimir K
>>
>> On 8/12/20 10:00 AM, Coleen Phillimore wrote:
>>>
>>>
>>> On 8/12/20 12:19 PM, Lois Foltan wrote:
>>>> On 8/12/2020 11:21 AM, Coleen Phillimore wrote:
>>>>> Summary: Changed some long declarations to uint64_t/int64_t or 
>>>>> unsigned int, depending on context.
>>>>>
>>>>> There are still 'long' declarations left in the code, but they 
>>>>> should be changed by developers when working in that code and not 
>>>>> as a blanket change.  I didn't change a couple of longs in 
>>>>> jfr/leakprofiler, for example.  These are the ones I changed though 
>>>>> with some explanation of why:
>>>>>
>>>>> src/hotspot/share/memory/filemap.hpp
>>>>>
>>>>> This can be negative so changed to int64_t.
>>>>>
>>>>> src/hotspot/share/runtime/mutex.cpp
>>>>>
>>>>> The PlatformMutex code takes jlong, which is signed, so that's why 
>>>>> I changed these to int64_t.
>>>>>
>>>>> runtime/interfaceSupport.inline.hpp
>>>>>
>>>>> These counters are actually intervals so I changed them to unsigned 
>>>>> int.
>>>>>
>>>>> src/hotspot/share/compiler/compileBroker.hpp
>>>>>
>>>>> _peak_compilation_time is signed because it is compared with jlong 
>>>>> which is signed.
>>>>> Same with total_compilation_time - elapsedTimer.milliseconds() 
>>>>> returns jlong.
>>>>>
>>>>> Tested with tier1-3.  Tier1 on linux-x64-debug, windows-x64-debug, 
>>>>> macosx-x64-debug, linux-aarch64-debug. Also built on: 
>>>>> linux-arm32,linux-ppc64le-debug,linux-s390x-debug,linux-x64-zero.
>>>>>
>>>>> open webrev at 
>>>>> http://cr.openjdk.java.net/~coleenp/2020/8235765.01/webrev
>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8235765
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>
>>>> Hi Coleen,
>>>>
>>>> Looks good.
>>>>
>>>> - runtime/sweeper.hpp
>>>>   This is the only file that I wondered why you changed long to 
>>>> int64_t for _total_nof_methods_reclaimed and 
>>>> _total_nof_c2_methods_reclaimed.  Note that the method 
>>>> NMethodSweeper::total_nof_methods_reclaimed returns an int. Could 
>>>> both of these fields be changed to int instead?
>>>
>>> Hi Lois, Thank you for looking at this.  Unfortunately, this was an 
>>> outdated webrev, can you hit reload?  I changed these fields to be 
>>> uint64_t because they're never signed.  It's likely that the number 
>>> of methods is never greater than an int, but since it was long to 
>>> begin with, I kept 64 bit until someone decides an 'int' is better. 
>>> Since number_of_codecache_sweeps is uint64_t, which seems like a lot 
>>> too, there could be that many nmethods reclaimed.  I retested with 
>>> windows just now to be sure.
>>>
>>> Thanks,
>>> Coleen
>>>
>>>
>>>>
>>>> Thanks,
>>>> Lois
>>>
>