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

Tony Printezis tprintezis at twitter.com
Fri Apr 29 14:13:26 UTC 2016


Ramki,

Thanks for the feedback. Latest webrev:

http://cr.openjdk.java.net/~tonyp/8155257/webrev.2/

I rephrased the three comments in concurrentMarkSweepGeneration.cpp before the calls to {start,stop}TrackingPromotions(). The logic should be the same.

Tony

On April 28, 2016 at 9:48:26 PM, Srinivas Ramakrishna (ysr1729 at gmail.com) wrote:

(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 be
1099   // called at the end of the main ParNew parallel phase to collapse
1100   // the promoted object lists. Given that we don't want promoted
1101   // objects to be tracked in future phases (e.g., during reference
1102   // 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:
But leaving them as is fine too.
reviewed!
-- ramki
2133   // Promotion tracking should be disabled at the end of the ParNew
2134   // parallel phase....
perhaps:
// When using ParNew, promoted object tracking would already have been disabled, however, ...

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 ParNew
1098   // parallel phase to collapse the promoted object lists. Given that
1099   // we don't want promoted objects to be tracked in future phases
1100   // (e.g., during reference processing) we also disable promote
1101   // 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 should
2134   // have already been disabled. However, it sanity checks that the
2135   // 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


-----

Tony Printezis | JVM/GC Engineer / VM Team | Twitter

@TonyPrintezis
tprintezis at twitter.com

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20160429/e5c29582/attachment.html>


More information about the hotspot-gc-dev mailing list