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

Jon Masamitsu jon.masamitsu at oracle.com
Thu Nov 1 19:20:38 UTC 2012


  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) 
after a young collection so this change should go into

  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.


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