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

Fri Apr 29 18:18:09 UTC 2016

ah, ok, in that case we should be good. I'll look at the update webrev
later today.
Thanks for catching these issues and fixing them!

-- ramki

On Fri, Apr 29, 2016 at 11:01 AM, Tony Printezis <tprintezis at twitter.com>
wrote:

> 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 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:
>>
>> But leaving them as is fine too.
>>
>> reviewed!
>>
>> -- ramki
>>
>> 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, ...
>>
>>
>> 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
>>>
>>>
>> -----
>>
>> 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/41b5755f/attachment.htm>