RFR: 8061630: G1 iterates over JNIHandles two times

[Bengt Rutisson]

Hi Erik,

On 2014-10-21 16:47, Erik Helin wrote:
> Hi all,
>
> this patch removes a redundant call to JNIHandles::weak_oops_do. The 
> call to JNIHandles::weak_oops_do can be removed because is it also 
> called by ReferenceProcessor::process_discovered_references via
> G1CollectedHeap::process_discovered_references, see 
> ReferenceProcessor::process_phaseJNI.
>
> Both calls to JNIHandles::weak_oops_do uses the same kind of closure for
> checking if an oop alive, G1STWIsAliveClosure. However, 
> JNIHandles::weak_oops_do in g1CollectedHeap.cpp gets passed an 
> G1KeepAliveClosure, whereas the call to JNIHandles::weak_oops_do in 
> referenceProcessor.cpp gets passed an G1CopyingKeepAliveClosure.
>
> Looking at JNIHandleBlock::weak_oops_do, one can see that the 
> `keep_alive` closure will only be called on the root if the `is_alive` 
> closure returns true for the value of the handle. If the 
> G1STWIsAliveClosure returns true, that means the value is either 
> outside the collection set or the value has been forwarded. Since the 
> root (JNIHandle) is outside the G1 heap, G1CopyingKeepAliveClosure 
> will use a G1ParCopyClosure (see _copy_non_heap_obj_cl in 
> G1CopyingKeepAliveClosure). Therefore, if we want to remove the 
> G1KeepAliveClosure phase, we must ensure ourselves that 
> G1ParCopyClosure and G1KeepAliveClosure do the same thing for a 
> JNIHandle root with a value that is either outside the collection set or
> already forwarded.
>
> If the value is outside the collection set and not humongous, then both
> G1ParCopyClosure and G1STWIsAliveClosure do nothing. If the value is
> humongous, then both closures calls 
> G1CollectedHeap::set_humongous_is_live. If the value is in the 
> collection set, then G1STWIsAliveClosure assumes it has been forwarded 
> and updates the root. G1ParCopyClosure will also update the root, 
> since the value already has been forwarded.
>
> Due to the above, G1ParCopyClosure already does everything 
> G1KeepAliveClosure does for JNIHandles, so there is no need to iterate 
> over them again using G1KeepAliveClosure.
>
> Webrev:
> http://cr.openjdk.java.net/~ehelin/8061630/webrev.00/

Looks good.

Thanks for the detailed explanation. It really helped to understand this 
change!

Bengt

>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8061630
>
> Testing:
> JPRT
>
> Thanks,
> Erik