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

David Holmes david.holmes at oracle.com
Mon Aug 17 01:54:15 UTC 2020 

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