<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Thanks, Jon.<br>
<br>
Sangheon<br>
<br>
<br>
<div class="moz-cite-prefix">On 04/01/2016 02:16 PM, Jon Masamitsu
wrote:<br>
</div>
<blockquote cite="mid:56FEE51E.6090208@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
Still looks good.<br>
<br>
Jon<br>
<br>
<br>
<div class="moz-cite-prefix">On 4/1/2016 8:26 AM, sangheon wrote:<br>
</div>
<blockquote cite="mid:56FE930C.9020903@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Hi Tom,<br>
<br>
Thanks for reviewing this.<br>
<br>
<div class="moz-cite-prefix">On 04/01/2016 08:03 AM, Tom Benson
wrote:<br>
</div>
<blockquote cite="mid:56FE8DD1.7020506@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Hi Sangheon,<br>
This looks fine to me except for a couple of trivial points in
commandLineFlagConstraintsGC.cpp comments.<br>
<br>
Typo in "should" :<br>
<pre><span class="new">+ // ParGCCardsPerStrideChunk shoule be</span></pre>
</blockquote>
Fixed.<br>
<br>
<blockquote cite="mid:56FE8DD1.7020506@oracle.com" type="cite">
<pre><span class="new">
</span></pre>
<pre><span class="new">I think the "However," in this line is not needed, and confusing:</span>
</pre>
<span class="new"></span><span class="new">+ // from
CardTableModRefBSForCTRS::process_stride(). However,
ParGCStridesPerThread is already checked<br>
<br>
I'd probably say "Note that ParGCStridesPerThread ..."
instead.<br>
</span></blockquote>
I like your suggestion and fixed.<br>
<br>
webrev:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8152176/webrev.02">http://cr.openjdk.java.net/~sangheki/8152176/webrev.02</a><br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8152176/webrev.02_to_01">http://cr.openjdk.java.net/~sangheki/8152176/webrev.02_to_01</a><br>
<br>
Thanks,<br>
Sangheon<br>
<br>
<br>
<blockquote cite="mid:56FE8DD1.7020506@oracle.com" type="cite"><span
class="new"> <br>
Tom<br>
</span><br>
<br>
<div class="moz-cite-prefix">On 3/29/2016 6:42 PM, sangheon
wrote:<br>
</div>
<blockquote cite="mid:56FB04CA.1000305@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Thanks, Jon.<br>
<br>
Sangheon<br>
<br>
<br>
<div class="moz-cite-prefix">On 03/29/2016 02:13 PM, Jon
Masamitsu wrote:<br>
</div>
<blockquote cite="mid:56FAEFEE.5030705@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<br>
<br>
<div class="moz-cite-prefix">On 3/29/2016 12:07 PM,
sangheon wrote:<br>
</div>
<blockquote cite="mid:56FAD254.8040909@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Hi Jon,<br>
<br>
Thank you for reviewing this.<br>
<br>
<div class="moz-cite-prefix">On 03/29/2016 10:49 AM, Jon
Masamitsu wrote:<br>
</div>
<blockquote cite="mid:56FAC01E.20303@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Sangheon,<br>
<br>
I agree that we don't want to expose <span
class="new">CardTableModRefBS::_last_valid_index<br>
but I'd consider making </span><span class="new"><span
class="new">CardTableModRefBS::</span>cards_required()
public.<br>
That would remove the need to duplicate the
calculation and it seems<br>
a reasonable question to ask the card table
implementation how many<br>
cards it is going to require. I'll let other
disagree if they like.<br>
</span></blockquote>
Good idea and changed to 'public'.<br>
<br>
<blockquote cite="mid:56FAC01E.20303@oracle.com"
type="cite"><span class="new"> <br>
Add in the message a little more about why the value
is too large.<br>
<br>
</span><br>
<pre><span class="new"> 396 CommandLineError::print(verbose,</span>
<span class="new"> 397 "ParGCCardsPerStrideChunk (" INTX_FORMAT ")
<font color="#ff0000"> "is too large for the heap size and "</font>
"must be "</span>
<span class="new"> 398 "less than or equal to card table size (" SIZE_FORMAT ")\n",</span>
<span class="new"> 399 value, card_table_size);
</span></pre>
</blockquote>
Fixed.<br>
<br>
<blockquote cite="mid:56FAC01E.20303@oracle.com"
type="cite">
<pre><span class="new">
</span></pre>
<pre><span class="new">I think a little more information here would help future maintainers so</span>
<span class="new">instead of </span>
<span class="new"></span></pre>
<span class="new"></span>
<pre><span class="new">403 // If n_strides which is used with ParGCCardsPerStrideChunk is really large, we would face an overflow.</span></pre>
<span class="new"><br>
</span></blockquote>
I updated the comment.<br>
<br>
<blockquote cite="mid:56FAC01E.20303@oracle.com"
type="cite"><span class="new"> This statement can
overflow?<br>
</span><br>
<pre><span class="new"> 404 uintx n_strides = ParallelGCThreads * ParGCStridesPerThread;</span></pre>
</blockquote>
No.<br>
Because above statement is checked not to overflow from
ParGCStridesPerThreadConstraintFunc() which is called
before this constraint function. (added this explanation
as well)<br>
</blockquote>
<br>
Ok.<br>
<br>
<blockquote cite="mid:56FAD254.8040909@oracle.com"
type="cite"> <br>
Updated webrev:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8152176/webrev.01/">http://cr.openjdk.java.net/~sangheki/8152176/webrev.01/</a><br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8152176/webrev.01_to_00/">http://cr.openjdk.java.net/~sangheki/8152176/webrev.01_to_00/</a><br>
</blockquote>
<br>
Looks good.<br>
<br>
Jon<br>
<br>
<blockquote cite="mid:56FAD254.8040909@oracle.com"
type="cite"> <br>
Thanks,<br>
Sangheon<br>
<br>
<br>
<blockquote cite="mid:56FAC01E.20303@oracle.com"
type="cite"> <span class="new"><br>
Jon<br>
<br>
</span>
<div class="moz-cite-prefix">On 3/28/2016 3:50 PM,
sangheon wrote:<br>
</div>
<blockquote cite="mid:56F9B548.3000504@oracle.com"
type="cite">Hi all, <br>
<br>
Could I have some reviews for this change? <br>
<br>
As very large value of ParGCCardsPerStrideChunk flag
would result in an overflow, we need a constraint
function after memory initialization. And the
function will check the flag to be less than of
equal to the size of card table and not to make an
overflow with other stride factors(ParallelGCThreads
and ParGCStridesPerThread). <br>
<br>
CR: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8152176">https://bugs.openjdk.java.net/browse/JDK-8152176</a>
<br>
Webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8152176/webrev.00">http://cr.openjdk.java.net/~sangheki/8152176/webrev.00</a>
<br>
Testing: JPRT, all runtime/commandline JTREG tests
on all platforms <br>
<br>
Thanks, <br>
Sangheon <br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>