RFR: 8155257: ParNew/CMS: Clean up promoted object tracking
jon.masamitsu at oracle.com
Thu Apr 28 20:47:18 UTC 2016
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.
> On April 28, 2016 at 4:33:44 PM, Tony Printezis
> (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?
>> On April 28, 2016 at 1:20:55 PM, Jon Masamitsu
>> (jon.masamitsu at oracle.com <mailto:jon.masamitsu at oracle.com>) wrote:
>>> Changes look good. Couple of small points.
>>> 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
>>> 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
>>> 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
>>> sanity check you can use? Comments sometimes get lost in the fog of
>>> 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 :-).
>>> 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:
>>>> 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 Printezis | JVM/GC Engineer / VM Team | Twitter
>>>> tprintezis at twitter.com <mailto:tprintezis at twitter.com>
>> Tony Printezis | JVM/GC Engineer / VM Team | Twitter
>> tprintezis at twitter.com <mailto:tprintezis at twitter.com>
> Tony Printezis | JVM/GC Engineer / VM Team | Twitter
> tprintezis at twitter.com <mailto:tprintezis at twitter.com>
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-gc-dev