RFR: 8039244: Don't use UINT32_FORMAT and INT32_FORMAT when printing uints and ints in the GC code

[Stefan Karlsson]

On 2014-04-04 23:58, Peter B. Kessler wrote:
> On 04/04/14 12:07, Stefan Karlsson wrote:
>> Vladimir,
>>
>> On 2014-04-04 17:58, Vladimir Kozlov wrote:
>>> Stefan,
>>>
>>> Could you explain more why you do this? Bug report does not explain 
>>> why you need this.
>>> The main reason we use *_FORMAT macros is because different 
>>> platforms behave differently when we use normal format's specifiers.
>>
>> First, *INT32_FORMAT isn't technically correct for printing ints. 
>> Though I agree that it probably works for most (all?) platforms 
>> HotSpot is run on.
>
> If INT32_FORMAT and UINT32_FORMAT are not technically correct, then 
> fix them.  That's a separate issue.  As Vladimir says, the whole point 
> of those macros is to localize the changes needed to run on platforms 
> where the native %d and %u don't work the way we expect them to.

INT32_FORMAT should be used for int that are exactly 32 bits, e.g. 
jints. If we really want to use *_FORMAT to print ints, we should 
introduce a INT_FORMAT. But _that_ is a separate issue.

>
>> Second, and probably more important, we usually use %u and %d to 
>> print ints, not UINT32_FORMAT and INT32_FORMAT. By changing these few 
>> occurrences in the GC code our print code gets more uniform.
>>
>> This is what you get if you grep after these format specifiers:
>>
>> $ grep -r "%d\|%u" src/ | wc
>>      3209   24297  353527
>>
>> $ grep -r "INT32_FORMAT" src/ | wc
>>        71     517    7841
>>
>> $ grep -r "%d\|%u" src/share/vm/memory/ 
>> src/share/vm/gc_implementation/ | wc
>>       426    3274   55241
>>
>> $ grep -r "INT32_FORMAT" src/share/vm/memory/ 
>> src/share/vm/gc_implementation/ | wc
>>        20     151    2603
>
> You don't seem to distinguish between code that uses native "int" and 
> "unsigned int" types (loop counters in tracing code, generation 
> numbers, region indexes, etc.) versus types that are the result of VM 
> configuration (sizes, offsets, field offsets, or calculations we need 
> to fit in 32 bits, etc.).
>
> You don't seem to distinguish between debugging code that might have 
> been thrown together without much thought versus logging code that we 
> want to be consistent across platforms.

I just wanted to make a point that we really don't use UINT32_FORMAT and 
INT32_FORMAT to print 'unsigned int' and 'int'. There are extremely few 
instances that does, and they should have followed the style we already 
have in place to print ints.

Just take a look at one of the few gclog_or_tty->print_cr changes I did:

http://cr.openjdk.java.net/~stefank/8039244/webrev.02/src/share/vm/gc_implementation/g1/heapRegionRemSet.cpp.frames.html

   432   if (G1TraceHeapRegionRememberedSet) {
  433     gclog_or_tty->print_cr("Table for [" PTR_FORMAT "...): card %d 
(cache = %d)",
  434                   hr()->bottom(), from_card,
  435                   FromCardCache::at((uint)tid, cur_hrs_ind));
  436   }

we already use %d in that method to print:

   492         if (G1TraceHeapRegionRememberedSet) {
  493           gclog_or_tty->print_cr("   [tid %d] sparse table entry "
  494                         "overflow(f: %d, t: %d)",
  495                         tid, from_hrs_ind, cur_hrs_ind);
  496         }

>
> The other use of the *_FORMAT macros is for printing pointers, sizes, 
> offsets, intx and uintx, jlong, etc.  It would be nice to be able to 
> use *_FORMAT macros consistently, which is another argument for the 
> whole family of them.

I don't argue against using them. I wouldn't go and use %p to print 
pointers, for example.

>
> I agree that the types are not always used consistently in the HotSpot 
> code.  But I'd rather we added information to the system rather than 
> removing it.

I want us to be consistent with our format specifier for the GC code 
(and hopefully for the entire HotSpot code base in the future).

Please start a new discussion if you want to setup a policy that HotSpot 
should use INT_FORMAT and UINT_FORMAT instead of %d and %u

StefanK

>
> I'm not a JDK-9 reviewer, so this is not a review.
>
>             ... peter
>
>> thanks,
>> StefanK
>>
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 4/4/14 12:59 AM, Stefan Karlsson wrote:
>>>> Please, review this patch to the GC code to change usages of 
>>>> UINT32_FORMAT and INT32_FORMAT to %u and %d when uints and
>>>> ints are used.
>>>>
>>>> While doing this change I found, and changed, a couple of places 
>>>> where we used UINT32_FORMAT to print variables that
>>>> were less than 4 bytes.
>>>>
>>>> webrev: http://cr.openjdk.java.net/~stefank/8039244/webrev.00/
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8039244
>>>>
>>>> thanks,
>>>> StefanK
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20140405/1574520b/attachment.htm>