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

[Jon Masamitsu]

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