RFR(S): 7098282: G1: assert(interval >= 0) failed: Sanity check, referencePolicy.cpp: 76

[John Cuthbertson]

Hi Bengt,

Thanks for the review. Replies inline....

On 10/10/11 04:26, Bengt Rutisson wrote:
>
> Hi John,
>
> Your change looks good, but I would actually like to go one step further.
>
> I would prefer to regard your new  field in ReferenceProcessor as the 
> master and just consider j.l.r.Softreference.clock a cached copy for 
> fast access in the j.l.r.Softreference.get() method. This is mostly a 
> conceptual change and does not necessarily need to lead to any major 
> code changes to your suggested fix. But the concept can be made 
> clearer by doing some refactoring:
>
> * Rename the field ReferenceProcessor::_shadow_soft_ref_clock. A 
> suggestion is to call it ReferenceProcessor::_current_soft_ref_timestamp
>
> *  Change the code in ReferenceProcessor::init_statics() to:
>
>   45 void ReferenceProcessor::init_statics() {
>   46   _current_soft_ref_timestamp = os::javaTimeMillis();
>   47   // Update the copy in the j.l.r.SoftReference class.
>   48   
> java_lang_ref_SoftReference::set_clock(_current_soft_ref_timestamp);
>
> * Similarly, ReferenceProcessor::update_soft_ref_master_clock() can be 
> turned inside-out wrt _current_soft_ref_timestamp and 
> SoftReference.clock.

I initially had the code this way (though I don't like the timestamp 
name - timestamp is too overloaded IMO) until it was pointed out that 
someone could, in fact, access/modify the Java field using some 
unorthodox manner. Since the current code uses the value currently in 
the Java field, and since that value can be modified, maintaining 
compatibility with existing behavior dictates that the value in Java 
class is the master value and the value in the ReferenceProcessor class 
is a convenient copy. Not breaking existing behavior removes a possible 
argument against backporting this fix into hs22.
>
> About the update in ReferenceProcessor::enable_discovery(). To me it 
> sounds a bit unnecessary to guard against Unsafe or reflection 
> manipulation of the private j.l.r.Softreference.clock field. The field 
> is clearly documented as "updated by the garbage collector". IMHO, if 
> anybody else updates that value they are on their own. In fact, I 
> don't see how we can guarantee that anything works correctly with 
> References if someone else is manipulating private fields in the 
> Reference classes. I know I am a bit naive when it comes to what Java 
> developers will do...

Agreed. In reality the situation will probably never come up. But 
currently there is nothing to prevent a user accesing/changing the value 
and manipulating the clearing policy directly.
>
> Finally, could we find a more suitable name for the "clock" parameter 
> to the should_clear_reference(oop p, jlong clock) methods? Maybe 
> "current_time" or "current_timestamp"?

Clock comes from the static field "clock" in the SoftReference class. I 
don't want to use "timestamp" as there is a "timestamp" instance field 
in the SoftRefence class. If the parameter name was "timestamp" then 
someone could reasonably assume that passing the value in the timestamp 
field from the SoftReference object was reasonable and correct. That is 
not the case. Better alternatives could be "timestamp_clock" (from the 
comment in the SoftReference.java file), "soft_ref_clock", or 
"soft_ref_timestamp_clock", but IMO "clock" is the important part of the 
name.

JohnC
>
> Thanks,
> Bengt
>
>
> On 2011-10-07 19:39, John Cuthbertson wrote:
>> Hi Everyone,
>>
>> Can I have a couple of volunteers review the code changes for this 
>> CR? The webrev can be found at: 
>> http://cr.openjdk.java.net/~johnc/7098282/webrev.0/
>>
>> Description:
>> This assert was being tripped because, during reference discovery in 
>> an evacuation pause, the value of the SoftReference master clock 
>> field became zero and so the timestamp interval was negative. The 
>> reason why the clock was zero was that another thread was in the 
>> process of copying the klass mirror for the SoftReference class (in 
>> which the statics are allocated). To resolve this issue I am 
>> maintaining a static shadow copy of the value of the SoftReference 
>> class' master clock and I use shadow copy in reference discovery and 
>> processing. This shadow copy is set when the Java field 
>> SoftReference::clock is set as well as a couple of strategic places 
>> (when discovery is enabled and right before processing the discovered 
>> soft references) in case the value of the Java field was modified 
>> using reflection or Unsafe access.
>>
>> Testing: The failing test case with all collectors, the GC test suite 
>> with all collectors.
>>
>> Thanks,
>>
>> JohnC
>