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

[Bengt Rutisson]

On 2014-04-23 12:25, Mikael Gerdin wrote:
> 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.

Looks good to me too.

I'll push this with:

Contributed-by: michal at frajt.eu

Thanks,
Bengt

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