RFR(M): 7009266: G1: assert(obj->is_oop_or_null(true )) failed: Error

[Tom Rodriguez]

c1_LIRGenerator.cpp:

you can't write the branches as you've done them.  The register allocator only understands block based control flow and the special case of CodeStubs.  It's a limitation of the LIR that unfortunately isn't checked for by any existing asserts.  It will work until the allocator decides to place a spill in the middle of your control flow.  So you either need to write the guard in the high level IR during parsing or you need to implement it in assembly in a stub on the side.  You should also try to constant fold some of these tests by hand since the LIR won't do that for you and in 99.99% of the cases they won't be needed.

library_call.cpp:

Can you put in the some guards to skip the code generation in cases where it's obviously not needed?  Otherwise we're generating a bunch of goo during parsing that we'll just have to optimize away.  Something like:

TypeX* otype = offset->find_intptr_t_type();
if (otype != NULL && otype->is_con() && otype->get_con() != java_lang_ref_Reference::_reference_offset) {
  // Constant offset but not the reference_offset so just return
  return;
}
TypeOopPtr* btype = base_oop->isa_oop_ptr();
if (btype != NULL && btype->isa_ary_ptr()) {
  // Array type so nothing to do
  return;
}

will filter most uses out completely.  Testing for TypeInstPtr would be slightly more complicated though I think something like

if (byte->isa_instr_ptr() &&
    !byte->isa_inst_ptr()->klass->is_subtype_of(env()->Reference_klass()) &&
    !env()->Reference_klass()->is_subtype_of(byte->isa_instptr()->klass()) {
  return;
}

would work.  Actually I don't think you should be testing against reference_type since we can't statically optimize those checks.  A regular instance of check can be optimized away if have good type information for the object.  Other than the constant formation it's no more expensive than the check you wrote.

tom

On Mar 28, 2011, at 10:35 AM, John Cuthbertson wrote:

> Hi Everyone,
> 
> A new webrev with changes based upon comments from Tom can be found at: http://cr.openjdk.java.net/~johnc/7009266/webrev.4/.
> 
> The latest changes include inserting a suitably guarded barrier call in case the referent field of a Reference object is being read/fetched using JNI, reflection, or Unsafe.
> 
> Thanks,
> 
> JohnC
> 
> On 3/11/2011 5:34 PM, John Cuthbertson wrote:
>> Hi Everyone,
>> 
>> I'm looking for a few of volunteers to review the changes that fix this assertion failure. The latest changes can be found at: http://cr.openjdk.java.net/~johnc/7009266/webrev.3/ and include changes based upon earlier internal reviews. The earlier changes are also on cr.openjdk.java.net for reference.
>> 
>> Background:
>> The G1 garbage collector includes a concurrent marking algorithm that makes use of snapshot-at-the-beginning or SATB. With this algorithm the GC will mark all objects that are reachable at the start of marking; objects that are allocated since the start of marking are implicitly considered live. In order to populate the "snapshot" of the object graph that existed at the start of marking, G1 employs a write barrier. When an object is stored into another object's field the write-barrier records the previous value of that field as it was part of the "snapshot" and concurrent marking will trace the sub-graph that is reachable from this previous value.
>> 
>> Unfortunately, in the presence of Reference objects, SATB might not be sufficient to mark a referent object as live. Consider that, at the start of marking, we have a weakly reachable object i.e. an object where the only pointer to that object. If the referent is obtained from the Reference object and stored to another object's field (making the referent now strongly reachable and hence live) the G1 write barrier will record the field's previous value but not the value of the referent.
>> 
>> If the referent object is strongly reachable from some other object that will be traced by concurrent marking, _or_ there is a subsequent assignment to the field where we have written the referent (in which case we record the previous value - the referent - in an SATB buffer) then the referent will be marked live. Otherwise the referent will not be marked.
>> 
>> That is the issue that was causing the failure in this CR. There was a Logger object that was only reachable through a WeakReference at the start of concurrent marking. During marking the Logger object is obtained from the WeakReference and stored into a field of a live object. The G1 write barrier recorded the previous value in the field (as it is part of the snapshot at the start of marking). Since there was no other assignment to the live object's field and there was no other strong reference to the Logger object, the Logger object was not marked. At the end of concurrent marking the Logger object was considered dead and the link between the WeakReference and the Logger was severed by clearing the referent field during reference processing.
>> 
>> To solve this (entirely in Hotspot and causing a performance overhead for G1 only) it was decided that the best approach was to intrinsify the Reference.get() method in the JIT compilers and add new interpreter entry points so that the value in the referent field will be recorded in an SATB buffer by the G1 pre-barrier code.
>> 
>> The changes for Zero and the C++ interpreters are place holder routines but should be straight forward to implement.
>> 
>> None of the individual changes is large - they are just well distributed around the JVM. :)
>> 
>> Testing: white box test; eyeballing the generated compiled and interpreter code; the failing Kitchensink big-app on x86 (32/64 bit), sparc (32/64 bit), Xint, Xcomp (client and server), with and without G1; the GC test suite with and without G1; and jprt.
>> 
>> Thanks and regards,
>> 
>> JohnC
>