RFR: 8144908: Remove apply_to_weak_ref_discovered_field override for UpdateRSOopClosure

[Jon Masamitsu]

Thanks Kim and Stefan for the explanation.

Change looks good.

Jon


On 12/09/2015 01:20 AM, Stefan Johansson wrote:
>
>
> On 2015-12-08 22:16, Kim Barrett wrote:
>> On Dec 8, 2015, at 3:34 PM, Jon Masamitsu <jon.masamitsu at oracle.com> 
>> wrote:
>>> Stefan,
>>>
>>> At line 41 in "share/vm/oops/instanceRefKlass.inline.hpp"
>>>
>>>     38  void 
>>> InstanceRefKlass::oop_oop_iterate_ref_processing_specialized(oop 
>>> obj, OopClosureType* closure,
>>> Contains& contains) {
>>>     39    T* disc_addr = 
>>> (T*)java_lang_ref_Reference::discovered_addr(obj);
>>>     40    if (closure->apply_to_weak_ref_discovered_field()) {
>>>     41      Devirtualizer<nv>::do_oop(closure, disc_addr);
>>>     42    }
>>>
>>> you're saying that the "next" field of "obj" is
>>> non-null?  Did you try adding a guarantee to that
>>> effect and run your testing (without your change,
>>> of course)?
>> If next is NULL then, <em>for the specific closure in question</em>, 
>> discovered must also be NULL here.  There is no point in applying 
>> closure to the NULL-valued discovered field.  There are other 
>> closures for which that relationship between next and discovered 
>> doesn’t hold, but we think it will turn out that none of them have 
>> apply_to_weak_ref_discovered_field returning true.
>>
>> Note that if next is non-NULL, then the closure will be applied to 
>> discovered in the normal path, and there is no need for this special 
>> case forced application (and is in fact duplicating work at present, 
>> which could be a bug on its own, except we think the special case is 
>> going away).
>>
>>
> As Kim say, for this specific closure the assumption holds, but it's 
> not true for all closures. Some time ago I did test-runs with extra 
> debug code to see which closures were applied with discovered field 
> set but next field NULL. The two closures that showed up was:
> FilterOutOfRegionClosure
> G1ParPushHeapRSClosure
>
> They both have apply_to_weak_ref_discovered_field returning true, but 
> to me it is not clear that they need it. My goal is to remove all 
> other use of apply_to_weak_ref_discovered_field first and then attack 
> those two. Either by somehow showing that the special case is no 
> longer needed or by handling the discovered field (or actually the 
> discovered list) some other way.
>

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