RFR: 8155257: ParNew/CMS: Clean up promoted object tracking

Fri Apr 29 01:48:25 UTC 2016

(Resent from my gmail id: as my twitter email id isn't registered with the
openjdk lists; please pardon duplicates.)

Looks good to me too. Minor remarks regarding the documentation comments:

1098   // The par_oop_since_save_marks_iterate_done() method should
be1099   // called at the end of the main ParNew parallel phase to
collapse1100   // the promoted object lists. Given that we don't want
promoted1101   // objects to be tracked in future phases (e.g., during
reference1102   // processing) we disable promoted object tracking.

Perhaps just say:

// Because card-scanning has been completed, further promotions, if any,

// e.g. during reference processing, will not need to keep track of
promoted objects.

Also instead of:

// Enable promotion tracking for the main parallel ParNew phase.

perhaps:

// We track promoted objects to allow card-scanning to skip them.

and instead of:

2133   // Promotion tracking should be disabled at the end of the
ParNew2134   // parallel phase....

perhaps:

// When using ParNew, promoted object tracking would already have been
disabled, however, ...

But leaving them as is fine too.

reviewed!

-- ramki

On Thu, Apr 28, 2016 at 2:06 PM, Tony Printezis <tprintezis at twitter.com>
wrote:

> Thanks Jon! New webrev here:
>
> http://cr.openjdk.java.net/~tonyp/8155257/webrev.1/
>
> Compared to the previous one, I only changed the comments before the two
> calls to stopTrackingPromotions(). The logic should be the same. Will also
> run another set of tests overnight.
>
> Tony
>
> On April 28, 2016 at 5:02:17 PM, Jon Masamitsu (jon.masamitsu at oracle.com)
> wrote:
>
>
>
> On 04/28/2016 01:51 PM, Tony Printezis wrote:
>
> Jon (CCing Ramki),
>
> Re: comment 2: Actually, it looks as if we have to call
> stopPromotionTracking in the epilogue. The prologue / epilogue are called
> irrespective of the type of GC. So they are also called for Full GCs which
> won’t call the method to tear down the lists. I think it’d be safer to just
> call stopPromotionTracking() in the epilogue, as I had it before, with an
> amended comment?
>
>
> That sounds good. I like the symmetry of putting the stopPromotionTracking
> in the epilogue.
>
> And yes, I'll run some addition tests when the latest patch comes out.
>
> Jon
>
>
> Tony
>
> On April 28, 2016 at 4:33:44 PM, Tony Printezis (tprintezis at twitter.com)
> wrote:
>
> Hi Jon,
>
> Thanks for looking at it!
>
> comment 1 : “this method” refers to the enclosing method (i.e.,
> par_oop_since_save_marks_iterate_done()). I’ll clarify. (I also spotted a
> typo in the comment! I’ll fix that too.)
>
> comment 2 : You’re right, actually. Good suggestion. I thought it might be
> awkward because of the ParNew-CMS interaction going through the generation
> abstractions. But in this code I’m already in the CMS generation and I can
> access the promoInfo directly. So I’ll just assert that tracking is off and
> the list is empty.
>
> I’ll post a new webrev in a bit. FWIW, overnight testing didn’t reveal any
> issues (and I’ll run it again given the “stop tracking” -> “sanity checks”
> change). Any chance you can also run some tests when I post the new webrev
> just in case?
>
> Tony
>
> On April 28, 2016 at 1:20:55 PM, Jon Masamitsu (jon.masamitsu at oracle.com)
> wrote:
>
> Tony,
>
> Changes look good.  Couple of small points.
>
>
> http://cr.openjdk.java.net/~tonyp/8155257/webrev.0/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp.frames.html
>
> 1097   // This method should be called at the end of the main ParNew1098   // parallel phase to collapse the promoted object lists. Given that1099   // we don't want promoted objects to be tracked in future phases1100   // (e.g., during reference processing) we also disable promote1101   // object tracking here.
>
> The placement of this block comment was slightly confusing.  I wasn't
> sure if "This method ..." applied specirfically  to the
> stopTrackingPromotions()
> until the last part "also disable ...".   Would it be better placed before
> the method.
>
> 2132   // Also reset promotion tracking in par gc thread states.2133   // I don't think this is really needed, as promotion tracking should2134   // have already been disabled. However, it sanity checks that the2135   // promotion lists are empty so I think it's helpful to leave it in.
>
> The comment makes clear you intent but  is there a simpler and more direct
> sanity check you can use?  Comments sometimes get lost in the fog of code
> churn and calling a method just for sanity checkinf seems wasteful and
> confusing
> (Why is stopTrackingPromotions() call twice?  Is there something that the
> first all didn't do? Yes, you comment explains it but I have to read the
> comment.)
> If you agree you can fix it at a later time.  If you disagree, I'll
> silently forget about it :-).
>
> Jon
>
> On 04/27/2016 01:40 PM, Tony Printezis wrote:
>
> Small changes to clean up when promoted object tracking is enabled /
> disabled and when tearing down the promoted object lists is done:
>
> http://cr.openjdk.java.net/~tonyp/8155257/webrev.0/
>
> I also had to amend an assert which was too strong and removed the
> worker_id argument from the stopTrackingPromotions() method as it was
> actually not used.
>
> I’ll run more testing overnight.
>
> Tony
>
> -----
>
> Tony Printezis | JVM/GC Engineer / VM Team | Twitter
>
> @TonyPrintezis
> tprintezis at twitter.com
>
>
> -----
>
> Tony Printezis | JVM/GC Engineer / VM Team | Twitter
>
> @TonyPrintezis
> tprintezis at twitter.com
>
> -----
>
> Tony Printezis | JVM/GC Engineer / VM Team | Twitter
>
> @TonyPrintezis
> tprintezis at twitter.com
>
>
> -----
>
> Tony Printezis | JVM/GC Engineer / VM Team | Twitter
>
> @TonyPrintezis
> tprintezis at twitter.com
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160428/afc0de00/attachment.htm>