[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-gc-dev
Subject: =?utf-8?b?UmU6IFJlcXVlc3QgZm9yIHJldmlldzogODAzODI2NTogQ01TOiBlbmFi?= =?utf-8?b?bGUgdGltZSBiYXNlZCB0c
From: "Michal Frajt" <michal () frajt ! eu>
Date: 2014-04-23 9:40:59
Message-ID: N4H9KB$C329D4FE433F13909E50401B5DDCDE87 () frajt ! eu
[Download RAW message or body]
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@frajt.eu
Komu: bengt.rutisson@oracle.com
Kopie: mikael.gerdin@oracle.com, hotspot-gc-dev@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@openjdk.java.net
> Komu: "Mikael Gerdin" mikael.gerdin@oracle.com, hotspot-gc-dev@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
> >
[Attachment #3 (text/html)]
<div class="xam_msg_class">
<div >Hi Mikael,<br><br>Find a new webrev which is addressing your small (mostly \
stylistic) comments. If you still find something missing we can quickly create new \
webrev. <br><br>Here is a new webrev:<br> <a target="_blank" \
class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/%7Ebrutisso/8038265/webrev.01/">http://cr.openjdk.java.net/~brutisso/8038265/webrev.01/</a><br>
<br>
Here is a diff between the old and the new webrev:<br>
<a target="_blank" class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/%7Ebrutisso/8038265/webrev.00-01.diff/">http://cr.openjdk.java.net/~brutisso/8038265/webrev.00-01.diff/</a><br><br>Best \
regards,<br>Michal<br><br> <div><span >Od</span><span >: "Michal Frajt" \
michal@frajt.eu</span></div> <div><span >Komu</span><span >: \
bengt.rutisson@oracle.com</span></div> <div><span >Kopie</span><span >: \
mikael.gerdin@oracle.com, hotspot-gc-dev@openjdk.java.net</span></div> <div><span \
>Datum</span><span >: Wed, 16 Apr 2014 16:36:38 +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 class="xam_msg_class">
<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>
</div>
</div>
</div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic