RFR(S): 7189971: Implement CMSWaitDuration for non-incremental mode of CMS
Jon Masamitsu
jon.masamitsu at oracle.com
Tue Dec 4 22:17:58 UTC 2012
I hope I haven't missed replies to my questions below but
sometimes things get lost in my in box. Was there
a reply?
Jon
On 11/01/12 12:20, Jon Masamitsu wrote:
>
> http://cr.openjdk.java.net/~johnc/7189971/webrev.0/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepThread.cpp.frames.html
>
>
> 327 double start_time = os::elapsedTime();
> 328 double end_time = start_time + (t_millis / 1000.0);
>
>
> Can these names be changes to start_time_secs and end_time_secs or
> something like that. That would remind people that the assumption is
> that os::elapsedTime() is returning seconds and, if a change to
> call something else is needed, that it should return seconds or
> changed to seconds.
>
> Also the 1000.0 should be changes to something like
> MILLISECS_PER_SEC. There isn't such a constant yet but
> globalDefinitions.hpp has
>
> const jlong NANOSECS_PER_SEC = CONST64(1000000000);
> const jint NANOSECS_PER_MILLISEC = 1000000;
>
> so adding one would fit.
>
> Is this really a modification that should be made in
> sleepBeforeNextCycle()?
> It seems like the benefit is that the initial mark be started
> (possibly) soon
> after a young collection so this change should go into
> checkpointRootsInitial()?
>
> 354 clear_CMS_flag(CMS_cms_wants_token);
> 355 assert(!CMS_flag_is_set(CMS_cms_has_token |
> CMS_cms_wants_token),
> 356 "Should not be set");
>
> Should the CMS_flag be set to CMS_cms_has_token after 356? Maybe if this
> code was implemented in checkpointRootsInitial() it would not have to
> worry about the token.
>
> Can you substitute "!_should_terminate" for "true" here?
>
> 337 while (true) {
>
>
>
> Can you add a 32 bit counter inside the loop and if the
> counter ever overflows, print out a warning message?
> It really should never happen but the warning message
> makes it easier to diagnose in the field. And you can
> ignore this if I'm being paranoid.
>
> Jon
>
>
>
> On 10/29/12 09:37, John Cuthbertson wrote:
>> Hi Everyone,
>>
>> Can I have a couple of volunteers review this change that was
>> submitted by Michal Frajt? The webrev can be found at:
>> http://cr.openjdk.java.net/~johnc/7189971/webrev.0/
>>
>> In the original patch, Michal was returning (if the CMS thread was
>> terminating or if a foreground collection was scheduled) after
>> setting the CMS flag. Michal incorporated my suggestion to change
>> that and return before setting the CMS flag.
>>
>> I'm not an expert in the inner workings of CMS so I'd appreciate
>> further reviews from those more familiar with CMS' operation.
>>
>> Testing:
>> Functional testing was performed using the GC test suite in both
>> incremental and non-incremental modes with various values of
>> CMSWaitDuration and checking the log files. Additional tests: nsk,
>> jprt.
>>
>> Thanks,
>>
>> JohnC
More information about the hotspot-gc-dev
mailing list