<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<br>
<div class="moz-cite-prefix">On 04/07/2016 11:22 AM, sangheon wrote:<br>
</div>
<blockquote cite="mid:5706A572.9030706@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
Hi Jon,<br>
<br>
<div class="moz-cite-prefix">On 04/07/2016 10:36 AM, Jon Masamitsu
wrote:<br>
</div>
<blockquote cite="mid:57069A8B.90304@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<font face="Times New Roman, Times, serif">Sangheon,<br>
<br>
Thanks for the changes. They look good. A couple<br>
of minor suggestions below.<br>
<br>
Jon<br>
</font><br>
<div class="moz-cite-prefix">On 04/06/2016 09:29 PM, sangheon
wrote:<br>
</div>
<blockquote cite="mid:5705E220.5010305@oracle.com" type="cite">Hi
Jon, <br>
<br>
Thank you for the review. <br>
<br>
On 04/05/2016 05:02 PM, Jon Masamitsu wrote: <br>
<blockquote type="cite">Sangheon, <br>
<br>
It occurs to me that if you created a function in CMS code
check_rescan_task_size_alignment() <br>
to do the check <br>
<br>
<blockquote type="cite">506 const size_t rescan_task_size =
cms->cmsSpace()->rescan_task_size(); <br>
507 const size_t alignment = CardTableModRefBS::card_size
* BitsPerWord; <br>
508 <br>
509 if ((size_t)round_to((intptr_t)rescan_task_size,
alignment) != rescan_task_size) { <br>
</blockquote>
<br>
then you won't have to expose the particular alignment
requirements in the <br>
constraint function. The code could look more like const
int HeapWordSize = sizeof(HeapWord);<br>
<br>
<br>
if ( check_rescan_task_size_alignment() ) { <br>
<br>
513 CommandLineError::print(verbose, <br>
514 "Rescan task size (" SIZE_FORMAT " = CMSRescanMultiple *
" <br>
515 "CardTableModRefBS::card_size_in_words * BitsPerWord)
must be " <br>
516 "aligned to CardTableModRefBS::card_size * BitsPerWord
(" SIZE_FORMAT "). " <br>
517 "Round-down value for CMSRescanMultiple is " SIZE_FORMAT
"\n", <br>
518 rescan_task_size, alignment, round_down_value); <br>
519 status = Flag::VIOLATES_CONSTRAINT; <br>
<br>
} <br>
<br>
<br>
You might be able to do something similar in the other
constraint function. <br>
</blockquote>
Your suggestion especially not exposing the alignment inspired
me to make it simpler. :) <br>
Task size is 'flag *CardTableModRefBS::card_size_in_words *
BitsPerWord' and we have to check the alignment with
CardTableModRefBS::card_size * BitsPerWord. As
CardTableModRefBS::card_size_in_words =
CardTableModRefBS::card_size / sizeof(HeapWord), if these
flags are a multiple of sizeof(HeapWord), the task size will
be aligned. <br>
<br>
<blockquote type="cite"> <br>
Also for <br>
<br>
472 if (value > addr_ergo_max) { <br>
473 CommandLineError::print(verbose, <br>
474 "%s (" SIZE_FORMAT ") must be " <br>
475 "less than or equal to ergonomic maximum (" SIZE_FORMAT
") " <br>
476 "based on start address corresponds to the old
generation of the Java heap\n", <br>
477 name, value, addr_ergo_max); <br>
478 return Flag::VIOLATES_CONSTRAINT; <br>
479 } <br>
<br>
Printing the addr_ergo_max might be confusing to the users
since it will be a very large <br>
number. Not sure what to do unless you can print a maximum
value of the flag based <br>
on the maximum heap size (instead of based on MAX_SIZE). <br>
</blockquote>
I agree that using max heap will be better, so I fixed. <br>
Considering your comment above (not exposing alignment
information as it is not nice), I had to add a new method to
get ergonomic maximum values for these task sizes. We can
check their constraint without this function, but can't print
out ergo max. Or have to use
CardTableModRefBS::card_size_in_words * BitsPerWord on its
calculation. <br>
<br>
Webrev: <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8152182/webrev.02">http://cr.openjdk.java.net/~sangheki/8152182/webrev.02</a>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8152182/webrev.02_to_01">http://cr.openjdk.java.net/~sangheki/8152182/webrev.02_to_01</a>
<br>
</blockquote>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8152182/webrev.02_to_01/src/share/vm/runtime/commandLineFlagConstraintsGC.cpp.frames.html">http://cr.openjdk.java.net/~sangheki/8152182/webrev.02_to_01/src/share/vm/runtime/commandLineFlagConstraintsGC.cpp.frames.html</a><br>
</blockquote>
</blockquote>
<blockquote cite="mid:5706A572.9030706@oracle.com" type="cite">
<blockquote cite="mid:57069A8B.90304@oracle.com" type="cite"> <br>
<pre><span class="changed"> 469 "based on reserved area corresponds to the old generation of the Java heap\n",</span>
<span class="changed">
"reserved area" is correct but perhaps not understandable for the average users.
Maybe something like
"which is based on the maximum size of the old generation of the Java heap\n"</span></pre>
</blockquote>
Fixed.<br>
<br>
<blockquote cite="mid:57069A8B.90304@oracle.com" type="cite">
<pre><span class="changed">
</span></pre>
<br>
<pre><span class="changed"> 492 value, sizeof(HeapWord));
>From share/vm/utilities/globalDefinitions.hpp
you could use HeapWorkSize
const int HeapWordSize = sizeof(HeapWord);</span></pre>
</blockquote>
I just repeated to use where it was calculated but I like is
better.<br>
<br>
Updated webrev:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8152182/webrev.03">http://cr.openjdk.java.net/~sangheki/8152182/webrev.03</a><br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8152182/webrev.03_to_02">http://cr.openjdk.java.net/~sangheki/8152182/webrev.03_to_02</a><br>
</blockquote>
<br>
Looks good. Thanks. for the changes.<br>
<br>
Jon<br>
<blockquote cite="mid:5706A572.9030706@oracle.com" type="cite"> <br>
Thanks,<br>
Sangheon<br>
<br>
<br>
<blockquote cite="mid:57069A8B.90304@oracle.com" type="cite">
<pre><span class="changed">
Jon
</span></pre>
<blockquote cite="mid:5705E220.5010305@oracle.com" type="cite">
<br>
Testing: JPRT, runtime/commandline JTREG on all platforms. <br>
<br>
Thanks, <br>
Sangheon <br>
<br>
<br>
<blockquote type="cite"> <br>
Ask if that's not clear. <br>
<br>
Jon <br>
<br>
<br>
<br>
On 4/5/2016 10:24 AM, sangheon wrote: <br>
<blockquote type="cite">Hi all, <br>
<br>
Please review this change for CMSRescanMultiple and
CMSConcMarkMultiple flags. <br>
<br>
Both flags are set by
"CardTableModRefBS::card_size_in_words * BitsPerWord *
flag" which potentially would make an overflow with their
maximum value without setting range. And these flags also
would make an arithmetic overflow when calculating with
the size and the start address of reserved area. In
addition, CMSRescanMultiple needs an alignment check. <br>
<br>
CR: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8152182">https://bugs.openjdk.java.net/browse/JDK-8152182</a>
<br>
Webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8152182/webrev.00">http://cr.openjdk.java.net/~sangheki/8152182/webrev.00</a>
<br>
Testing: JPRT, runtime/commandline JTREG for all platforms
<br>
<br>
Thanks, <br>
Sangheon <br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>