<div class="xam_msg_class">
<div >Hi Bengt,<br><br>We will provide you the new patch next week. All Mikael's comments will be addressed.<br><br>Best regards,<br>Michal<br><br>
<div><span >Od</span><span >: "hotspot-gc-dev" hotspot-gc-dev-bounces@openjdk.java.net</span></div>
<div><span >Komu</span><span >: "Mikael Gerdin" mikael.gerdin@oracle.com, hotspot-gc-dev@openjdk.java.net</span></div>
<div><span >Kopie</span><span >: </span></div>
<div><span >Datum</span><span >: Mon, 14 Apr 2014 17:24:16 +0200</span></div>
<div><span >Předmet</span><span >: Re: Request for review: 8038265: CMS: enable time based triggering of concurrent cycles</span></div>
<br>
<div>> </div><div>> Hi Michal,</div><div>> </div><div>> I think the change looks good modulo the comments that Mikael has. If </div><div>> you fix those I can help you with a new webrev and then push this change.</div><div>> </div><div>> Thanks,</div><div>> Bengt</div><div>> </div><div>> </div><div>> On 2014-04-01 11:52, Mikael Gerdin wrote:</div><div>> > Hi Michal,</div><div>> ></div><div>> > On Monday 31 March 2014 14.32.01 Michal Frajt wrote:</div><div>> >> Hi all,could you please review the following change we prepared together</div><div>> >> with Bengt? The change adds new CMSTriggerInterval flag which allows to</div><div>> >> invoke CMS collections periodically. The periodically running CMS</div><div>> >> collections are helping us with replacing the deprecated incremental CMS</div><div>> >> collector. We believe it might be useful for other customers currently</div><div>> >> using the CMS in the incremental mode.Bugs:</div><div>> >> https://bugs.openjdk.java.net/browse/JDK-8038265</div><div>> >></div><div>> >> Webrev:</div><div>> >> http://cr.openjdk.java.net/~brutisso/8038265/webrev.00/</div><div>> > If the addition of this functionality helps you replace iCMS then that's</div><div>> > great! I think this code change is simple and easy to understand.</div><div>> ></div><div>> > I just have some small (mostly stylistic) comments:</div><div>> ></div><div>> >   The other time output in PrintCMSInitiationStatistics seems to use      </div><div>> >        %3.7f as the format specifier instead of %g.</div><div>> ></div><div>> > 1515     gclog_or_tty->print_cr("cms_time_since_begin=%g",</div><div>> > stats().cms_time_since_begin());</div><div>> > 1516     gclog_or_tty->print_cr("cms_time_since_end=%g",</div><div>> > stats().cms_time_since_end());</div><div>> ></div><div>> ></div><div>> >      The HotSpot style is to have a space between if and the opening brace,</div><div>> >  can you please change the if here:</div><div>> ></div><div>> > 1588     if(stats().cms_time_since_begin() >= (CMSTriggerInterval / ((double)</div><div>> > MILLIUNITS))) {</div><div>> > 1589       if (Verbose && PrintGCDetails) {</div><div>> ></div><div>> >         And here:</div><div>> ></div><div>> > 1590         if(stats().valid()) {</div><div>> ></div><div>> >    We usually align the parameters on the second line with the opening     </div><div>> >        brace on the function call, something like:</div><div>> ></div><div>> > gclog_or_tty->print_cr(foo, bar,</div><div>> >                         stats().baz());</div><div>> ></div><div>> > 1591           gclog_or_tty->print_cr("CMSCollector: collect because of</div><div>> > trigger interval (time since last begin %g secs)",</div><div>> > 1592             stats().cms_time_since_begin());</div><div>> > 1593         } else {</div><div>> ></div><div>> >         You pass in cms_time_since_begin() but don't actually use that value    </div><div>> >        in the else branch.</div><div>> ></div><div>> > 1594           gclog_or_tty->print_cr("CMSCollector: collect because of</div><div>> > trigger interval (first collection)",</div><div>> > 1595             stats().cms_time_since_begin());</div><div>> > 1596         }</div><div>> > 1597       }</div><div>> ></div><div>> > /Mikael</div><div>> ></div><div>> ></div><div>> >> Thanks,</div><div>> >> Michal</div><div>> </div></div>
</div>