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

David Holmes david.holmes at oracle.com
Fri Aug 14 23:19:54 UTC 2020 

On 13/08/2020 10:20 pm, Coleen Phillimore wrote:
> 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).

Ok.

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

No "long" is 32-bit on all 32-bit platforms. So I would expect passing a 
64-bit value to a library function expecting a 32-bit argument should 
generate a compiler warning at best. The fact it doesn't is a concern. 
I'd be worried about the correctness of this code now.

David


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

More information about the hotspot-dev mailing list