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

[Michal Frajt]

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/

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
> > 



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20140423/2aaa2930/attachment.htm>