RFR: 8155257: ParNew/CMS: Clean up promoted object tracking
Jon Masamitsu
jon.masamitsu at oracle.com
Tue May 3 18:07:40 UTC 2016
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>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160503/981eeed3/attachment.htm>
More information about the hotspot-gc-dev
mailing list