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

Tony Printezis tprintezis at twitter.com
Thu Apr 28 20:51:35 UTC 2016


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?

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

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


More information about the hotspot-gc-dev mailing list