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

[Ramki Ramakrishna]

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

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

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
>