[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
