RFR: 8155257: ParNew/CMS: Clean up promoted object tracking
kirk at kodewerk.com
kirk at kodewerk.com
Tue May 3 18:30:14 UTC 2016
Hi Jon,
Are you able to calculate allocation rates?
Kind regards,
Kirk
> On May 3, 2016, at 8:07 PM, Jon Masamitsu <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/ <http://cr.openjdk.java.net/%7Etonyp/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 ( <mailto:ysr1729 at gmail.com>ysr1729 at gmail.com <mailto: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 < <mailto:tprintezis at twitter.com>tprintezis at twitter.com <mailto:tprintezis at twitter.com>> wrote:
>>>>>>>> Thanks Jon! New webrev here:
>>>>>>>>
>>>>>>>> <http://cr.openjdk.java.net/%7Etonyp/8155257/webrev.1/>http://cr.openjdk.java.net/~tonyp/8155257/webrev.1/ <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 ( <mailto:jon.masamitsu at oracle.com>jon.masamitsu at oracle.com <mailto: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 ( <mailto:tprintezis at twitter.com>tprintezis at twitter.com <mailto: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 ( <mailto:jon.masamitsu at oracle.com>jon.masamitsu at oracle.com <mailto:jon.masamitsu at oracle.com>) wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Tony,
>>>>>>>>>>>>
>>>>>>>>>>>> Changes look good. Couple of small points.
>>>>>>>>>>>>
>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Etonyp/8155257/webrev.0/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp.frames.html> <http://cr.openjdk.java.net/%7Etonyp/8155257/webrev.0/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp.frames.html>http://cr.openjdk.java.net/~tonyp/8155257/webrev.0/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp.frames.html <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/%7Etonyp/8155257/webrev.0/>http://cr.openjdk.java.net/~tonyp/8155257/webrev.0/ <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
>>>>>>>>>>>>> <mailto:tprintezis at twitter.com>tprintezis at twitter.com <mailto:tprintezis at twitter.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>> -----
>>>>>>>>>>>
>>>>>>>>>>> Tony Printezis | JVM/GC Engineer / VM Team | Twitter
>>>>>>>>>>>
>>>>>>>>>>> @TonyPrintezis
>>>>>>>>>>> <mailto:tprintezis at twitter.com>tprintezis at twitter.com <mailto:tprintezis at twitter.com>
>>>>>>>>>>>
>>>>>>>>>> -----
>>>>>>>>>>
>>>>>>>>>> Tony Printezis | JVM/GC Engineer / VM Team | Twitter
>>>>>>>>>>
>>>>>>>>>> @TonyPrintezis
>>>>>>>>>> <mailto:tprintezis at twitter.com>tprintezis at twitter.com <mailto:tprintezis at twitter.com>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>> -----
>>>>>>>>
>>>>>>>> Tony Printezis | JVM/GC Engineer / VM Team | Twitter
>>>>>>>>
>>>>>>>> @TonyPrintezis
>>>>>>>> <mailto:tprintezis at twitter.com>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/dbdea57c/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 496 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160503/dbdea57c/signature.asc>
More information about the hotspot-gc-dev
mailing list