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

[Mikael Gerdin]

Hi Michal,

On Wednesday 23 April 2014 11.40.59 Michal Frajt wrote:
> Hi Mikael,
> 
> Find a new webrev which is addressing your small (mostly stylistic)
> comments. If you still find something missing we can quickly create new
> webrev.
> 
> Here is a new webrev:
> 
>     http://cr.openjdk.java.net/~brutisso/8038265/webrev.01/
> 
> 
> 
>     Here is a diff between the old and the new webrev:
> 
>     http://cr.openjdk.java.net/~brutisso/8038265/webrev.00-01.diff/

The change looks good to me.
/Mikael

> 
> Best regards,
> Michal
> 
> 
> Od: "Michal Frajt" michal at frajt.eu
> Komu: bengt.rutisson at oracle.com
> Kopie: mikael.gerdin at oracle.com, hotspot-gc-dev at openjdk.java.net
> Datum: Wed, 16 Apr 2014 16:36:38 +0200
> Předmet: Re: Request for review: 8038265: CMS: enable time based triggering
> of concurrent cycles
> > Hi Bengt,
> > 
> > We will provide you the new patch next week. All Mikael's comments will be
> > addressed.
> > 
> > Best regards,
> > Michal
> > 
> > 
> > Od: "hotspot-gc-dev" hotspot-gc-dev-bounces at openjdk.java.net
> > Komu: "Mikael Gerdin" mikael.gerdin at oracle.com,
> > hotspot-gc-dev at openjdk.java.net Kopie:
> > Datum: Mon, 14 Apr 2014 17:24:16 +0200
> > Předmet: Re: Request for review: 8038265: CMS: enable time based
> > triggering of concurrent cycles> 
> > > 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