RFR: 8155257: ParNew/CMS: Clean up promoted object tracking
Jon Masamitsu
jon.masamitsu at oracle.com
Wed May 4 00:03:16 UTC 2016
On 5/3/2016 12:47 PM, Tony Printezis wrote:
> Hi Jon,
>
> Thanks again for evaluating the change. Yes, I’m expecting ParNew
> pause time improvements with this change but only if a) there’s a
> non-trivial amount of promotions during the ParNews and b) there are
> more than a couple of GC threads available. For benchmarks that run
> for a fixed amount of time, shorter pause times will of course mean
> more GCs during the timing interval.
>
> Do you need anything else from me?
Not at this point. I just need to look at some of the logs to be sure there
are no surprises.
Jon
>
> Tony
>
> On May 3, 2016 at 2:07:57 PM, Jon Masamitsu (jon.masamitsu at oracle.com
> <mailto:jon.masamitsu at oracle.com>) wrote:
>
>> Performance results are back. If anything I see a decrease in ParNew
>> pause times
>> and an increase in the number of ParNew GC's. Is that the expected
>> results? Faster
>> GC's so more time for the benchmark to make progress (so more
>> allocations and more
>> GC's)?
>>
>> Jon
>>
>> On 5/2/2016 2:59 PM, Tony Printezis wrote:
>>> Sounds good, thanks Jon!!!
>>>
>>> On May 2, 2016 at 5:35:14 PM, Jon Masamitsu
>>> (jon.masamitsu at oracle.com <mailto:jon.masamitsu at oracle.com>) wrote:
>>>
>>>>
>>>>
>>>> On 05/02/2016 08:02 AM, Tony Printezis wrote:
>>>>> Jon,
>>>>>
>>>>> I also didn’t like having to add a new method to Generation just
>>>>> for ParNew (but note that ParNew is the only GC that uses the
>>>>> existing par_oop_since_save_marks_iterate_done() method). On the
>>>>> other hand, I also didn’t like having to enable / disable
>>>>> something in the epilogue / prologue unnecessarily (i.e., when the
>>>>> GC is not going to be done by ParNew).
>>>>>
>>>>> Anyway, it looks as if the original fix where we enable / disable
>>>>> promotion tracking in the prologue / epilogue seems the least
>>>>> controversial. :-) So, let’s go with that. Latest is webrev.2. and
>>>>> I withdraw webrev.3.suggestion.
>>>>
>>>> I'll do some performance runs with .2 and push if there are no
>>>> regressions.
>>>>
>>>> Jon
>>>>
>>>>>
>>>>> Tony
>>>>>
>>>>> On April 29, 2016 at 2:49:11 PM, Jon Masamitsu
>>>>> (jon.masamitsu at oracle.com <mailto:jon.masamitsu at oracle.com>) wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On 4/29/2016 8:03 AM, Tony Printezis 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?
>>>>>>
>>>>>> Tony,
>>>>>>
>>>>>> I can see how nicely this works with the implementation but
>>>>>> adding the new method
>>>>>> par_oop_since_save_marks_iterate_start() to Generation causes me
>>>>>> some concern.
>>>>>> If there were another collector that used
>>>>>> par_oop_since_save_marks_iterate_start(),
>>>>>> it would make more sense to me.
>>>>>>
>>>>>> Going with your previous version where the CMSGen
>>>>>> epilogue/prologue is
>>>>>> responsible for this does make sense from the point of view that
>>>>>> it is done
>>>>>> by the generation that needs it.
>>>>>>
>>>>>> Alternatively, we could just all admit that ParNew only works
>>>>>> with CMS and
>>>>>> make the _old_gen in ParNewGeneration a
>>>>>> ConcurrentMarkSweepGeneration*
>>>>>> and avoid adding the method to Generation. This might not work
>>>>>> so easily with
>>>>>> full GC's and ScavengeBeforeFullGC though.
>>>>>>
>>>>>> I think this is more important for you guys then for us so don't
>>>>>> hesitant to
>>>>>> push for the solution you like.
>>>>>>
>>>>>> Jon
>>>>>>>
>>>>>>> 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 <mailto:tprintezis at twitter.com>) wrote:
>>>>>>>
>>>>>>>> Ramki,
>>>>>>>>
>>>>>>>> Thanks for the feedback. Latest webrev:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~tonyp/8155257/webrev.2/
>>>>>>>> <http://cr.openjdk.java.net/%7Etonyp/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 <mailto:tprintezis at twitter.com>
>>>>>>>>
>>>>>>> -----
>>>>>>>
>>>>>>> Tony Printezis | JVM/GC Engineer / VM Team | Twitter
>>>>>>>
>>>>>>> @TonyPrintezis
>>>>>>> tprintezis at twitter.com <mailto:tprintezis at twitter.com>
>>>>>>>
>>>>>>
>>>>> -----
>>>>>
>>>>> Tony Printezis | JVM/GC Engineer / VM Team | Twitter
>>>>>
>>>>> @TonyPrintezis
>>>>> tprintezis at twitter.com <mailto:tprintezis at twitter.com>
>>>>>
>>>>
>>> -----
>>>
>>> Tony Printezis | JVM/GC Engineer / VM Team | Twitter
>>>
>>> @TonyPrintezis
>>> tprintezis at twitter.com <mailto:tprintezis at twitter.com>
>>>
>>
> -----
>
> Tony Printezis | JVM/GC Engineer / VM Team | Twitter
>
> @TonyPrintezis
> tprintezis at twitter.com <mailto:tprintezis at twitter.com>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160503/17fa2a99/attachment.htm>
More information about the hotspot-gc-dev
mailing list