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

Coleen Phillimore coleen.phillimore at oracle.com
Mon Aug 17 14:06:54 UTC 2020 

Thanks David.
Coleen

On 8/16/20 9:54 PM, David Holmes wrote:
> Looks good.
>
> Thanks,
> David
>
> On 15/08/2020 8:59 pm, Coleen Phillimore wrote:
>>
>>
>> On 8/14/20 7:58 PM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> I suggest just leaving the elfFile stuff alone. AFAICS the 
>>> underlying offset is 32-bit on 32-bit and 64-bit on 64-bit and on 
>>> the platforms we use ElfFile the use of "long" matches that (ILP32 
>>> and LP64). This code should probably be using some of the Elf types 
>>> directly, hidden by a typedef to hide the 32-bit versus 64-bit 
>>> nomenclature.
>>
>> Ok, I reverted elfFile.* also.
>>
>> open webrev at 
>> http://cr.openjdk.java.net/~coleenp/2020/8235765.03/webrev
>>
>> Thanks!
>> Coleen
>>>
>>> David
>>>
>>> On 15/08/2020 9:19 am, David Holmes wrote:
>>>> 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