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

Fri Apr 29 18:01:00 UTC 2016

Ramki,

Inline.

On April 29, 2016 at 1:52:01 PM, Srinivas Ramakrishna (ysr1729 at gmail.com) wrote:

(responding from my gmail because of list membership, but please keep my twitter id in cc so I know when to look :-)

Haven't yet looked at yr webrev, but just wanted to send this quick remark .... (inline below) ....

On Fri, Apr 29, 2016 at 8:03 AM, Tony Printezis <tprintezis at twitter.com> wrote:
Ramki and Jon,

I hate to confused things further. :-) I just realized that if only ParNew needs the promotion tracking to be enabled for the parallel workers, aren’t we better off just letting ParNew enable / disable it when it needs to instead of always doing so in the CMSGeneration prologue / epilogue?

.... Well, it's the non-contiguity of the CMS generation that requires promotion object tracking.
It doesn't matter if we have one worker thread or many. 

I understand. But please note that the CMS per-worker state which contains the per-worker PromotionInfo objects (note: CMS, not ParNew; i.e., the CMSParGCThreadState) is only used by ParNew as far as I can see. The serial code (which in this case is only used during serial ref processing, DefNew has been disabled for use with CMS in JDK 9) uses a separate PromotionInfo object (_promoInfo on CompactibleFreeListSpace) which is enabled / disabled separately (in CFLS::save_marks() and CFLS::gc_epilogue() resp.). 

Tony

Since promotion can happen into
dirty cards which are being scanned "concurrently" (i.e. interleaved in the case of a single
worker), and our closures do not like to double-scan objects, we need to prevent that by
specially marking promoted objects while card scanning is in progress.

More after I have looked at yr webrev (much later this PM)...

-- ramki

Here’s an alternative webrev for your consideration:

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

I think I prefer this approach but I’m OK with either. (And, yes, I’ll actually expand the comments I’ve marked as TODO if you prefer this one.)

Tony

On April 29, 2016 at 10:13:27 AM, Tony Printezis (tprintezis at twitter.com) wrote:

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

-----

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/20160429/edf91d78/attachment.htm>