RFR: 8139892: Allow G1CollectorPolicy to specify if reference processing should be enabled

[Erik Helin]

On 2015-11-18, Kim Barrett wrote:
> On Nov 18, 2015, at 4:32 PM, Tom Benson <tom.benson at oracle.com> wrote:
> > 
> > Hi Erik,
> > I'm getting out of my depth of knowledge of reference processing here, so will put this in the form of a question.  8^)  In addition to what you describe below and in JBS, the new code explicitly processes weak JNI handles if should_process_references() is false.  OK, that must be done regardless of whether processing normal ref objects.  (Perhaps a comment to this effect above the call to the new process_weak_jni_handles would be good.)
> > 
> > The code in ReferenceProcessor::process_discovered_references contains some ancient comments about the ordering of processing normal refs vs JNI refs, implying the latter must be last.  (Referring to the equally ancient JDK-4126360. )
> > 
> > This change seems to allow the order to be reversed, since JNI handles will be processed before references are, at some point in the future.  Is there an issue that code might be relying on this ordering?
> 
> That shouldn't be a problem.
> 
> Any objects that are the referents of a JNI weak reference and also
> the referent of some j.l.r.Reference will be treated as strongly
> reachable, because reference discovery was disabled and so the
> j.l.r.Reference referent fields were treated as strong rather than
> weak.
> 
> So at the decision between process_discovered_references and
> process_weak_jni_handles, there should not be any discovered
> references.  The selection between the two functions to call is
> perhaps an optimization to avoid some somewhat expensive nops in
> process_discovered_references when we know there aren't any to
> process.
> 
> >> On 2015-11-10 14:05, Erik Helin wrote: 
> >>> Hi all, 
> >>> 
> >>> this patch adds the method `bool should_process_references() const` to 
> >>> G1CollectorPolicy and also updates G1CollectedHeap to make use of it. 
> >>> This enables a G1CollectorPolicy to make decisions about whether 
> >>> references should be processed or not. 
> >>> 
> >>> Enhancement: 
> >>> https://bugs.openjdk.java.net/browse/JDK-8139892 
> >>> 
> >>> Webrev: 
> >>> http://cr.openjdk.java.net/~ehelin/8139892/webrev.00/ 
> 
> ------------------------------------------------------------------------------ 
> src/share/vm/gc/g1/g1CollectedHeap.cpp  
> 3812       if (g1_policy()->should_process_references()) {
> 3813         ref_processor_stw()->enable_discovery();
> 3814       }
> 
> I'd like discovery to be explicitly disabled if
> !should_process_references, to avoid any question about what the
> incoming state might be.

Agree, fixed.

> ------------------------------------------------------------------------------ 
> src/share/vm/gc/g1/g1CollectedHeap.cpp 
> 5185   if (g1_policy()->should_process_references()) {
> 5186     process_discovered_references(per_thread_states);
> 5187   } else {
> 5188     process_weak_jni_handles();
> 5189   }
> 
> How about calling ref_processor_stw()->verify_no_references_recorded()
> before calling process_weak_jni_handles?

Agree, fixed.

Please see new webrevs:
- full: http://cr.openjdk.java.net/~ehelin/8139892/webrev.01/
- inc: http://cr.openjdk.java.net/~ehelin/8139892/webrev.00-01/

Thanks,
Erik