Request for review: 8038265: CMS: enable time based triggering of concurrent cycles

[Bengt Rutisson]

Hi Michal,

I think the change looks good modulo the comments that Mikael has. If 
you fix those I can help you with a new webrev and then push this change.

Thanks,
Bengt


On 2014-04-01 11:52, Mikael Gerdin wrote:
> Hi Michal,
>
> On Monday 31 March 2014 14.32.01 Michal Frajt wrote:
>> Hi all,could you please review the following change we prepared together
>> with Bengt? The change adds new CMSTriggerInterval flag which allows to
>> invoke CMS collections periodically. The periodically running CMS
>> collections are helping us with replacing the deprecated incremental CMS
>> collector. We believe it might be useful for other customers currently
>> using the CMS in the incremental mode.Bugs:
>> https://bugs.openjdk.java.net/browse/JDK-8038265
>>
>> Webrev:
>> http://cr.openjdk.java.net/~brutisso/8038265/webrev.00/
> If the addition of this functionality helps you replace iCMS then that's
> great! I think this code change is simple and easy to understand.
>
> I just have some small (mostly stylistic) comments:
>
> 	The other time output in PrintCMSInitiationStatistics seems to use 	
> 	%3.7f as the format specifier instead of %g.
>
> 1515     gclog_or_tty->print_cr("cms_time_since_begin=%g",
> stats().cms_time_since_begin());
> 1516     gclog_or_tty->print_cr("cms_time_since_end=%g",
> stats().cms_time_since_end());
>
>
> 	The HotSpot style is to have a space between if and the opening brace,
> 	can you please change the if here:
>
> 1588     if(stats().cms_time_since_begin() >= (CMSTriggerInterval / ((double)
> MILLIUNITS))) {
> 1589       if (Verbose && PrintGCDetails) {
>
> 	And here:
>
> 1590         if(stats().valid()) {
>
> 	We usually align the parameters on the second line with the opening 	
> 	brace on the function call, something like:
>
> gclog_or_tty->print_cr(foo, bar,
>                         stats().baz());
>
> 1591           gclog_or_tty->print_cr("CMSCollector: collect because of
> trigger interval (time since last begin %g secs)",
> 1592             stats().cms_time_since_begin());
> 1593         } else {
>
> 	You pass in cms_time_since_begin() but don't actually use that value 	
> 	in the else branch.
>
> 1594           gclog_or_tty->print_cr("CMSCollector: collect because of
> trigger interval (first collection)",
> 1595             stats().cms_time_since_begin());
> 1596         }
> 1597       }
>
> /Mikael
>
>
>> Thanks,
>> Michal