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

[Tony Printezis]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160428/931875d5/attachment.htm>