<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 2014-04-04 23:58, Peter B. Kessler
wrote:<br>
</div>
<blockquote cite="mid:533F2B01.30500@Oracle.COM" type="cite">On
04/04/14 12:07, Stefan Karlsson wrote:
<br>
<blockquote type="cite">Vladimir,
<br>
<br>
On 2014-04-04 17:58, Vladimir Kozlov wrote:
<br>
<blockquote type="cite">Stefan,
<br>
<br>
Could you explain more why you do this? Bug report does not
explain why you need this.
<br>
The main reason we use *_FORMAT macros is because different
platforms behave differently when we use normal format's
specifiers.
<br>
</blockquote>
<br>
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.
<br>
</blockquote>
<br>
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.
<br>
</blockquote>
<br>
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.<br>
<br>
<blockquote cite="mid:533F2B01.30500@Oracle.COM" type="cite">
<br>
<blockquote type="cite">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.
<br>
<br>
This is what you get if you grep after these format specifiers:
<br>
<br>
$ grep -r "%d\|%u" src/ | wc
<br>
3209 24297 353527
<br>
<br>
$ grep -r "INT32_FORMAT" src/ | wc
<br>
71 517 7841
<br>
<br>
$ grep -r "%d\|%u" src/share/vm/memory/
src/share/vm/gc_implementation/ | wc
<br>
426 3274 55241
<br>
<br>
$ grep -r "INT32_FORMAT" src/share/vm/memory/
src/share/vm/gc_implementation/ | wc
<br>
20 151 2603
<br>
</blockquote>
<br>
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.).
<br>
<br>
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.
<br>
</blockquote>
<br>
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. <br>
<br>
Just take a look at one of the few gclog_or_tty->print_cr changes
I did:<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~stefank/8039244/webrev.02/src/share/vm/gc_implementation/g1/heapRegionRemSet.cpp.frames.html">http://cr.openjdk.java.net/~stefank/8039244/webrev.02/src/share/vm/gc_implementation/g1/heapRegionRemSet.cpp.frames.html</a><br>
<br>
432 if (G1TraceHeapRegionRememberedSet) {<br>
433 gclog_or_tty->print_cr("Table for [" PTR_FORMAT "...):
card %d (cache = %d)",<br>
434 hr()->bottom(), from_card,<br>
435 FromCardCache::at((uint)tid, cur_hrs_ind));<br>
436 }<br>
<br>
we already use %d in that method to print:<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
492 if (G1TraceHeapRegionRememberedSet) {<br>
493 gclog_or_tty->print_cr(" [tid %d] sparse table
entry "<br>
494 "overflow(f: %d, t: %d)",<br>
495 tid, from_hrs_ind, cur_hrs_ind);<br>
496 }<br>
<br>
<blockquote cite="mid:533F2B01.30500@Oracle.COM" type="cite">
<br>
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.
<br>
</blockquote>
<br>
I don't argue against using them. I wouldn't go and use %p to print
pointers, for example.<br>
<br>
<blockquote cite="mid:533F2B01.30500@Oracle.COM" type="cite">
<br>
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.
<br>
</blockquote>
<br>
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).<br>
<br>
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<br>
<br>
StefanK<br>
<br>
<blockquote cite="mid:533F2B01.30500@Oracle.COM" type="cite">
<br>
I'm not a JDK-9 reviewer, so this is not a review.
<br>
<br>
... peter
<br>
<br>
<blockquote type="cite">thanks,
<br>
StefanK
<br>
<br>
<blockquote type="cite">
<br>
Thanks,
<br>
Vladimir
<br>
<br>
On 4/4/14 12:59 AM, Stefan Karlsson wrote:
<br>
<blockquote type="cite">Please, review this patch to the GC
code to change usages of UINT32_FORMAT and INT32_FORMAT to
%u and %d when uints and
<br>
ints are used.
<br>
<br>
While doing this change I found, and changed, a couple of
places where we used UINT32_FORMAT to print variables that
<br>
were less than 4 bytes.
<br>
<br>
webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~stefank/8039244/webrev.00/">http://cr.openjdk.java.net/~stefank/8039244/webrev.00/</a>
<br>
bug: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8039244">https://bugs.openjdk.java.net/browse/JDK-8039244</a>
<br>
<br>
thanks,
<br>
StefanK
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>