<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sangheki/8152182/webrev.02">http://cr.openjdk.java.net/~sangheki/8152182/webrev.02</a>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sangheki/8152182/webrev.02_to_01">http://cr.openjdk.java.net/~sangheki/8152182/webrev.02_to_01</a>
<br>
</blockquote>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sangheki/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>
<br>
<pre><span class="changed"> 492 value, sizeof(HeapWord));
>From share/vm/utilities/globalDefinitions.hpp
you could use HeapWorkSize
const int HeapWordSize = sizeof(HeapWord);
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 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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sangheki/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>
</body>
</html>