<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi,<br>
<br>
Please look at the new webrev: <br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~eistepan/6407976/webrev.01">http://cr.openjdk.java.net/~eistepan/6407976/webrev.01</a><br>
<br>
Thank you!<br>
Jane<br>
<div class="moz-cite-prefix">On 15.04.2015 11:31, Evgeniya Stepanova
wrote:<br>
</div>
<blockquote cite="mid:552E21E1.3040009@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
Hi Igor, Stefan, Jesper<br>
<br>
Thank you very much for the reviews!<br>
<div class="moz-cite-prefix">On 15.04.2015 10:57, Stefan Karlsson
wrote:<br>
</div>
<blockquote cite="mid:552E19E5.1020103@oracle.com" type="cite">On
2015-04-14 21:51, Jesper Wilhelmsson wrote: <br>
<blockquote type="cite">Stefan Karlsson skrev den 14/4/15 14:05:
<br>
<blockquote type="cite"> <br>
<br>
On 2015-04-09 14:30, Evgeniya Stepanova wrote: <br>
<blockquote type="cite">Hi, <br>
<br>
Please review fix for JDK-6407976. <br>
In some source files GC worker threads numbers are being
stored as <br>
signed values and same values in other classes are being
stored as unsigned <br>
values. Fix changed GC worker thread number to be uint. <br>
<br>
bug: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-6407976">https://bugs.openjdk.java.net/browse/JDK-6407976</a>
<br>
webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eeistepan/6407976/webrev.00/">http://cr.openjdk.java.net/~eistepan/6407976/webrev.00/</a>
<br>
</blockquote>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eeistepan/6407976/webrev.00/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp.patch">http://cr.openjdk.java.net/~eistepan/6407976/webrev.00/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp.patch</a>
<br>
<br>
void G1CollectedHeap::reset_taskqueue_stats() { <br>
- const int n = workers()->total_workers(); <br>
- for (int i = 0; i < n; ++i) { <br>
+ const uint n = workers()->total_workers(); <br>
+ for (int i = 0; i < (int) n; ++i) { <br>
task_queue(i)->stats.reset(); <br>
} <br>
} <br>
<br>
Why do you have this (int) cast here? Didn't you change the
task_queue function <br>
with: <br>
- RefToScanQueue *task_queue(int i) const; <br>
+ RefToScanQueue *task_queue(uint i) const; <br>
<br>
</blockquote>
<br>
That (int) does not exist in the diffs or frame view of the
webrev, only in the patch. In the rest of the webrev 'i' in
the loop is uint. <br>
<br>
Jane, please make sure your webrev contains the latest version
of your change. For some reason old versions of some files
seems to be mixed with new ones in this webrev. <br>
</blockquote>
<br>
I wonder if this is yet another case of this bug: <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/CODETOOLS-7901115">https://bugs.openjdk.java.net/browse/CODETOOLS-7901115</a>
- webrev -r generates broken patch files <br>
<br>
</blockquote>
That's really strange. I'll double check patches next time to
prevent this happened again<br>
<blockquote cite="mid:552E19E5.1020103@oracle.com" type="cite">
<blockquote type="cite"> <br>
<blockquote type="cite"> <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eeistepan/6407976/webrev.00/src/share/vm/gc_implementation/shared/adaptiveSizePolicy.cpp.patch">http://cr.openjdk.java.net/~eistepan/6407976/webrev.00/src/share/vm/gc_implementation/shared/adaptiveSizePolicy.cpp.patch</a>
<br>
<br>
You should keep the parameter alignments: <br>
<br>
-int AdaptiveSizePolicy::calc_active_workers(uintx
total_workers, <br>
+uint AdaptiveSizePolicy::calc_active_workers(uintx
total_workers, <br>
uintx
active_workers, <br>
uintx
application_workers) { <br>
<br>
-int AdaptiveSizePolicy::calc_active_conc_workers(uintx
total_workers, <br>
+uint AdaptiveSizePolicy::calc_active_conc_workers(uintx
total_workers, <br>
uintx
active_workers, <br>
uintx
application_workers) { <br>
<br>
<br>
This goes for the other files as well. <br>
</blockquote>
</blockquote>
</blockquote>
Thank you, will fix<br>
<br>
<blockquote cite="mid:552E19E5.1020103@oracle.com" type="cite">
<blockquote type="cite">
<blockquote type="cite"> <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eeistepan/6407976/webrev.00/src/share/vm/runtime/vm_version.cpp.udiff.html">http://cr.openjdk.java.net/~eistepan/6407976/webrev.00/src/share/vm/runtime/vm_version.cpp.udiff.html</a>
<br>
<br>
As Igor mentioned, it would be good to only use uint: <br>
<br>
-int Abstract_VM_Version::_parallel_worker_threads = 0; <br>
+unsigned int Abstract_VM_Version::_parallel_worker_threads
= 0; <br>
</blockquote>
<br>
Looking at the rest of vm_version.cpp we can see that unsigned
int is consistently used throughout the file. I don't know
why, but maybe there is a reason. We should probably check
with runtime before introducing uint. In any case I don't
think this change should introduce uint in just a single
place. If we want this file to use uint that should be changed
throughout the file in a separate change. <br>
</blockquote>
<br>
OK. Let's use unsigned int then. <br>
</blockquote>
<br>
I also think it should be done with the separate rfr for all
sources. So I'll keep it "as is" for now.<br>
<br>
Thanks once again,<br>
Jane<br>
<blockquote cite="mid:552E19E5.1020103@oracle.com" type="cite"> <br>
Thanks, <br>
StefanK <br>
<br>
<blockquote type="cite"> <br>
Thanks, <br>
/Jesper <br>
<br>
<blockquote type="cite"> <br>
Thanks, <br>
StefanK <br>
<br>
<blockquote type="cite"> <br>
tested by running all hotspot tests on all platforms <br>
<br>
<br>
Thanks, <br>
Jane <br>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
<div class="moz-signature">-- <br>
<i>Evgeniya Stepanova</i></div>
</blockquote>
<br>
<div class="moz-signature">-- <br>
<i>Evgeniya Stepanova</i></div>
</body>
</html>