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

[Jon Masamitsu]

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 <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 
>> (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/~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 <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/20160428/5f4762b5/attachment.htm>