request for review (M): 4965777 GC changes to support use of discovered field for pending references

Y. S. Ramakrishna y.s.ramakrishna at oracle.com
Tue Sep 6 12:52:10 PDT 2011


[i left the embedded inlines below for the cores-lib folk
who may have missed part of the exchange because of non-membership of
the posters.]

Hi Stefan --

On 09/05/11 10:49, Stefan Karlsson wrote:
> Hi Ramki,
> 
> On 09/05/2011 07:29 PM, Ramki Ramakrishna wrote:
>>
>> Hi Stefan --
>>
>> Thanks for your prompt review!
>>
>> On 9/5/2011 5:05 AM, Stefan Karlsson wrote:
>>> Hi Ramki,
>>>
>>> Your changes seems to be based on my changes to remove the 
>>> sentinelRef and use a self-looping end in the discovered lists, so my 
>>> review is based on that.
>>
>> Yes, that's correct. I should have remembered to mention that in my email
>> to these review aliases.
>>
>>>
>>> referenceProcessor.cpp:
>>>
>>> Why do you want to change the variable name to next_d?
>>>   325   oop next_d = refs_list.head();
>>
>> To keep from confusing it with the next field's name. Just a naming 
>> convention.
>> (next_d == next discovered object) to keep confusion wrt "next" to a 
>> minimum
>> for the unwary reader.
>>
>>>
>>> This seems to be incorrect. The discovered list should never end with 
>>> NULL, since its self-looping.
>>>   348         java_lang_ref_Reference::set_discovered(obj, old); // 
>>> old may be NULL
>>
>> Yes, I considered that, which was one of the reasons I wanted to place 
>> my changes
>> on top of yours. However, it turns out that that protocol's only needed
>> for objects that will end up on the discovered lists maintained by GC.
>> Once pended, an object is never eligible to be discovered again
>> (recall the next!=NULL check during reference discovery by GC), so
>> the discovered field need not conform to the "never NULL" protocol
>> necessary for not-yet-pended objects (present on the discovered
>> lists of GC).
> 
> OK, I see your point. IMHO it would be prudent to use the same EOL 
> marker for the pending list as for the discovered lists and the queued 
> references (through the next field), just for consistency. But I'll not 
> stress this point.

I understand your concern. I will keep this in mind, as we watch any
bug tail, but at the moment I'm inclined not to change the treatment
of the pending list terminator in the same way as was done for the
discovered list terminator.

thanks again for your review!
-- ramki

> 
>>
>>>
>>>
>>> instanceRefKlass.cpp:
>>>
>>> Typo, discoveredd -> discovered
>>>   95     // In the case of older JDKs which do not use the discoveredd
>>>  175     // In the case of older JDKs which do not use the discoveredd
>>>  399     // In the case of older JDKs which do not use the discoveredd
>>
>> Fixed.
>>
>>>
>>> Why was the contains check moved?
>>>   207   if (!oopDesc::is_null(heap_oop) && contains(referent_addr)) {
>>
>> This was because the scanning should be based on containment of the
>> oop in the interval of interest, but discovery of  the oop is not. 
>> This is
>> really a somewhat pedantic change to clarify the intent of the 
>> contains check
>> rather than anything substantive, because in all of our current
>> reference processors, whenever contains is non-degenerate (i.e. where it
>> does not return true), discovery will never actually happen, so the two
>> forms are behaviourally equivalent relative to our current set of
>> allowed executions. I prefer the new form though because it clarifies
>> that the interval is only used for filtering the scan and not for 
>> other matters.
> 
> OK.
> 
> StefanK
> 
>>
>> Reading the code though (both old and new), it seems as though this 
>> could be
>> further cleaned up, but a clean-up that I prefer deferred to
>> a separate changeset.
>>
>>>
>>>
>>> Reference.java:
>>>
>>> The last element in the pending list should point back to itself.
>>>   102      *     pending:   next element in the pending list (or null 
>>> if last)
>>
>> See my point above regarding why that is not necessary.
>>
>> Let me know.
>> -- ramki
>>
>>>
>>>
>>> StefanK
>>>
>>> On 09/05/2011 11:16 AM, Ramki Ramakrishna wrote:
>>>>
>>>> CR's 4243978 and  4268317 have proposed to use the discovered field of
>>>> a java.lang.ref.reference object to link the objects in the pending 
>>>> list.
>>>> This requires changes in both GC and in the reference handler code.
>>>> The JVM adaptsso as to allow it to run either inside a newer JDK 
>>>> which uses the
>>>> discovered field to link the references or inside an older JDK which 
>>>> uses the
>>>> next field for that purpose. Although the JDK part of the changes are
>>>> also being submitted for review here, that part will be integrated 
>>>> as a separate
>>>> CR and changeset into the JDK repo following the integration of the 
>>>> JVM changes
>>>> into an appropriate version of the HotSpot (express) repos.
>>>>
>>>>   JVM webrev:  http://cr.openjdk.java.net/~ysr/4965777/hotspot/webrev/
>>>>   JDK webrev:   http://cr.openjdk.java.net/~ysr/4965777/jdk/webrev/
>>>>
>>>> Many thanks to Mandy Chung and John Coomes for advice and for
>>>> earlier reviews; and to Mandy for additional testing help.
>>>>
>>>> Thanks for any other reviews or comments.
>>>> -- ramki
>>>
> 


More information about the hotspot-gc-dev mailing list