<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hello.<br>
<br>
Here is updated webrev<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~vkempik/8016302/webrev.03/">http://cr.openjdk.java.net/~vkempik/8016302/webrev.03/</a><br>
<br>
<br>
<br>
Thanks, Vladimir.<br>
<div class="moz-cite-prefix">On 03.10.2013 22:41, Jon Masamitsu
wrote:<br>
</div>
<blockquote cite="mid:524DBA6D.5090307@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<br>
<div class="moz-cite-prefix">On 10/3/13 8:37 AM, Vladimir Kempik
wrote:<br>
</div>
<blockquote cite="mid:524D8F38.2060304@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
Hello<br>
<br>
<div class="moz-cite-prefix">On 03.10.2013 18:57, Jon Masamitsu
wrote:<br>
</div>
<blockquote cite="mid:524D85F5.6030507@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Evkempik/8016302/webrev.02/src/share/vm/gc_implementation/g1/concurrentG1Refine.cpp.udiff.html">http://cr.openjdk.java.net/~vkempik/8016302/webrev.02/src/share/vm/gc_implementation/g1/concurrentG1Refine.cpp.udiff.html</a><br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<pre> for (int i = _n_threads - 1; i >= 0; i--) {
</pre>
Did you consider using "uint i" instead of "int i"? Maybe<br>
adding an assert<br>
<br>
assert(_n_threads > 0, ...)<br>
<br>
In order to avoid the cast<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<pre><span class="new">+ ConcurrentG1RefineThread* t = new ConcurrentG1RefineThread(this, next, worker_id_offset, (uint)i);
</span><span class="new"></span></pre>
</blockquote>
But then i >= 0 will always be true. The loop will look
much uglier if I transform it to check against MAX_UINT<br>
</blockquote>
<br>
OK.<br>
<br>
<blockquote cite="mid:524D8F38.2060304@oracle.com" type="cite"> <br>
<blockquote cite="mid:524D85F5.6030507@oracle.com" type="cite">
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Evkempik/8016302/webrev.02/src/share/vm/gc_implementation/g1/concurrentMark.cpp.udiff.html">http://cr.openjdk.java.net/~vkempik/8016302/webrev.02/src/share/vm/gc_implementation/g1/concurrentMark.cpp.udiff.html</a><br>
<br>
I used to seeing all parameters on the same line or each
parameter<br>
on its own line. Instead of<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<pre><span class="new">+ G1NoteEndOfConcMarkClosure g1_note_end(_g1h, &local_cleanup_list,</span>
&old_proxy_set,
&humongous_proxy_set,
&hrrs_cleanup_task);
</pre>
this style.<br>
<br>
<pre><span class="new">+ G1NoteEndOfConcMarkClosure g1_note_end(_g1h,
&local_cleanup_list,</span>
&old_proxy_set,
&humongous_proxy_set,
&hrrs_cleanup_task);
</pre>
I know, not really your code originally but since you touched
it, I thought I<br>
would mention it.<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Evkempik/8016302/webrev.02/src/share/vm/gc_implementation/g1/dirtyCardQueue.cpp.udiff.html">http://cr.openjdk.java.net/~vkempik/8016302/webrev.02/src/share/vm/gc_implementation/g1/dirtyCardQueue.cpp.udiff.html</a><br>
<br>
</blockquote>
<br>
I will update webrev to include these changes.<br>
<br>
</blockquote>
Thanks.<br>
<br>
<blockquote cite="mid:524D8F38.2060304@oracle.com" type="cite"> <br>
<blockquote cite="mid:524D85F5.6030507@oracle.com" type="cite">
Is this cast necessary?<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<pre><span class="new">+ (uint) worker_i);
</span><span class="new"></span></pre>
</blockquote>
<br>
worker_i is defined as size_t in that function, it's almost
same, but not always (on every platform we have), and that code
is compiled with -WError, so I've decided to make it clear for
compiler and not produce any warnings.<br>
</blockquote>
<br>
Ok.<br>
<br>
Jon<br>
<blockquote cite="mid:524D8F38.2060304@oracle.com" type="cite"> <br>
<blockquote cite="mid:524D85F5.6030507@oracle.com" type="cite">
Rest looks good.<br>
<br>
Jon<br>
<pre><span class="new"></span></pre>
<br>
<br>
<pre><span class="new"></span></pre>
<br>
<div class="moz-cite-prefix">On 10/3/13 3:32 AM, Vladimir
Kempik wrote:<br>
</div>
<blockquote cite="mid:524D47CA.4040708@oracle.com" type="cite">Hello
<br>
<br>
Can I have two reviewers to looks at it please ? <br>
<br>
Thanks, Vladimir. <br>
On 26.09.2013 11:41, Per Liden wrote: <br>
<blockquote type="cite">Looks good! <br>
<br>
cheers, <br>
/Per <br>
<br>
On 2013-09-25 12:47, Vladimir Kempik wrote: <br>
<blockquote type="cite">Hello <br>
<br>
Thanks for comments, updated webrev - <a
moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Evkempik/8016302/webrev.02/">http://cr.openjdk.java.net/~vkempik/8016302/webrev.02/</a>
<br>
<br>
<br>
On 24.09.2013 17:22, Thomas Schatzl wrote: <br>
<blockquote type="cite">Hi, <br>
<br>
On Thu, 2013-09-19 at 16:22 +0200, Vladimir Kempik
wrote: <br>
<blockquote type="cite">Hello <br>
<br>
Thanks for comments, here is updated webrev - <br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Evkempik/8016302/webrev.01/">http://cr.openjdk.java.net/~vkempik/8016302/webrev.01/</a>
<br>
<br>
</blockquote>
- the comments in
DirtyCardQueueSet::mut_process_buffer() should be <br>
adapted that we use uints now. They talk of worker_i
of -1. <br>
<br>
E.g. <br>
<br>
130 // If worker_i is not -1 then the thread has
already claimed <br>
131 // a par_id. We make note of it using the
already_claimed value <br>
<br>
- in G1GCPhaseTimes::print_stats() we need to use
UINT32_FORMAT instead <br>
of %d as the printf() specifier for uints. <br>
<br>
- same in ScanRSClosure::printCard(). <br>
<br>
Thanks, <br>
Thomas <br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>