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

[Michal Frajt]

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/20140416/b2b1e348/attachment.htm>