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

Thu Aug 13 12:20:47 UTC 2020

On 8/13/20 12:07 AM, David Holmes wrote:
> Hi Coleen,
>
> And it seemed so simple when you started :)

Oh gosh no, this sort of change is never simple!
>
> 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. :)

Yes, another sort of hard-to-impossible to completely fix RFE.  I really 
think we should just fix individual occurrences as they're found and 
discourage new uses.  Ideally we should translate all j* types to their 
appropriate int types once they get passed into the VM implementation code.
>
>
> 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; }

Rats missed one.  I wish the compiler would have complained about this.  
Nothing calls this function so I removed it (_filesize is used directly).
>
> 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.

These files aren't compiled on windows.  I compiled this on arm32. I 
think there "long" is 64 bits.

#if !defined(_WINDOWS) && !defined(__APPLE__)

Thanks,
Coleen

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