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

[Jon Masamitsu]

On 4/28/2016 1:33 PM, Tony Printezis 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?

I ran some tests with

http://cr.openjdk.java.net/~tonyp/8155257/webrev.1/

and no new failures.  :-)

Jon

PS.  "no new failures" means that any failures that occurred are known
failures with CR's already assigned.  FYI, they were all serviceability
failures and probably would have occurred across the different GC's.


>
> 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>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160429/f3324018/attachment.htm>