RFR(S): 7189971: Implement CMSWaitDuration for non-incremental mode of CMS

[Jon Masamitsu]

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