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

[Coleen Phillimore]

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.

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
>>