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

[Vladimir Kozlov]

Looks good.

I filed compiler's RFE to clean our types:
https://bugs.openjdk.java.net/browse/JDK-8251505

Thanks,
Vladimir K

On 8/12/20 3:56 PM, Coleen Phillimore wrote:
> 
> 
> On 8/12/20 4:43 PM, 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.
>>>
>>> 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.
>>>
>>> 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.
> 
> This is a more limited attempt at stomping out 'long' in the shared code.
> 
> open webrev at http://cr.openjdk.java.net/~coleenp/2020/8235765.02/webrev
> 
> thanks,
> Coleen
>>
>> 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
>>>>
>>
>