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

[Bengt Rutisson]

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.

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

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"?

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