RFR (XS): 8200362: G1Mux2Closure should disable implicit oop verification (original) (raw)

Stefan Karlsson stefan.karlsson at oracle.com
Thu Mar 29 10:19:57 UTC 2018

On 2018-03-29 11:25, Stefan Johansson wrote:

On 2018-03-29 11:20, Stefan Karlsson wrote:

On 2018-03-29 11:02, Thomas Schatzl wrote: Hi Stefan, On Thu, 2018-03-29 at 09:52 +0200, Stefan Johansson wrote: Hi Thomas,

On 2018-03-28 17:27, Thomas Schatzl wrote: Hi all,

can I have reviews for this small change that fixes some annoyance when debugging with G1? In particular, G1Mux2Closure is used by verification to verify and in case of error print out g1 specific error messages. However, since G1Mux2Closure is an OopClosure, it is actually wrapped by NoHeaderExtendedOopClosure in oopiterate* calls. ExtendedOopClosure has its own verification that is very generic, and may trigger in the same situations as G1Mux2Closure. Disable this implicit verification for G1Mux2Closure so that we always get the g1 specific error messages. CR: https://bugs.openjdk.java.net/browse/JDK-8200362 Webrev: http://cr.openjdk.java.net/~tschatzl/8200362/webrev/ Nice change, always good to get detailed information. I think you need to change the code using the G1Mux2Closure as well. On line 666 in heapRegion.cpp: G1Mux2Closure mux(&vlcl, &vrcl); obj->oopiteratenoheader(&mux); This should now be a normal oopiterate(&mux) to make sure the right verification is done. Fixed. Also looked through other closures used for verification. http://cr.openjdk.java.net/~tschatzl/8200362/webrev.0to1 (diff) http://cr.openjdk.java.net/~tschatzl/8200362/webrev.1 (full) Looks good. I like that we get rid of usages of oopiteratenoheader. Maybe we should remove the other usages in G1, as a separate RFE. I like the sound of that, maybe something for Leo to look at. Any history or reason why we have these?

Some years ago ExtendedOopClosure and OopClosure were only one class, the OopClosure. The ExtendedOopClosure was created and all GC specific code needed for the oop_iterate functions were pulled out from OopClosure and put into ExtendedOopClosure.

ExtendedOopClosure is very GC centric, while OopClosure is a much leaner API to apply a closure to an oop* (or an narrowOop*).

The iterate_oop_no_header (and NoHeaderExtendedOopClosure) was intended to simplify non-GC JVM code to apply an OopClosure to all oop* of an object, without having to understand the ExtendedOopClosure class. The no_header name is legacy from the Permgen when we sometimes walked the "header", the klass pointer. Today no_header means "don't care about metadata pointers, or other GC stuff".

We've recently realized that instanceRefKlass::oop_oop_iterate is broken when we run GCs with load barriers and concurrent reference processing, and non-GC code applies a normal, strong, load barrier to the referent field. That strong load barrier will try to use the stale pointer, and most likely cause crashes. Therefore, oop_iterate_no_header is problematic, and should probably be completely removed. The GCs can deal with this by simply implementing ExtendedOopClosure and use oop_iterate, just like we do in this patch.

StefanK

StefanJ StefanK

Thomas

More information about the hotspot-gc-dev mailing list