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