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

[Vladimir Kozlov]

Hi Coleen,

I think it is safe to use 'uint' (uint32_t) for all counts in sweeper.

What is a story about using int64_t vs jlong? And others *_t vs j* types.

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.

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
>