<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
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 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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~eistepan/6407976/webrev.00/">http://cr.openjdk.java.net/~eistepan/6407976/webrev.00/</a>
<br>
</blockquote>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~eistepan/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 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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~eistepan/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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~eistepan/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>
</body>
</html>