RFR (S) 8190891: Clean up G1 barrier code in compiler interface (ci)

[Kim Barrett]

> On Nov 8, 2017, at 2:58 PM, coleen.phillimore at oracle.com wrote:
> 
> 
> Kim,
> 
> Thank you for revewing.
> 
> On 11/8/17 2:28 PM, Kim Barrett wrote:
>>> On Nov 7, 2017, at 7:38 PM, coleen.phillimore at oracle.com wrote:
>>> 
>>> Summary: consolidate gc barrier code in ci
>>> 
>>> See bug and comments for more details.
>>> 
>>> Tested with mach5 tier1-5 on linux x64 and iwndows x64.  Also tested StressRedefineWithoutBytecodeModification in test/jdk/java/lang/instrument which fails without the g1 barriers.
>>> 
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8190891.01/webrev
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8190891
>>> 
>>> Thanks,
>>> Coleen
>> ------------------------------------------------------------------------------
>> src/hotspot/share/ci/ciInstanceKlass.cpp
>>   55 static void ensure_metadata_alive(oop metadata_holder) {
>> ...
>>   60   if (metadata_holder != NULL) {
>> 
>> When would we ever get a ciInstanceKlass for a Klass with a NULL
>> holder?  That seems like a situation that shouldn't ever happen, and
>> this should be an assert instead.  And there is an assert here
> 
> When it's the NULL class loader, associated with ClassLoaderData::the_null_class_loader_data().   This class loader is never unloaded.
>> 
>> 75   assert(get_instanceKlass()->is_loaded(), "must be at least loaded");
>> 
>> that dominates the ensure_metadata_alive call that is similarly
>> suggestive.  Though I'm not sure whether is_loaded can be true if the
>> klass was loaded and then later had its holder die and be cleared.
> 
> Not sure I understand this comment.  The InstanceKlass must be in the "loaded" _init_state to get to this code, but the marking for ensure_metadata_alive and creating an object prevents it from being unloaded after this point.
>> 
>> And can the call to get_object at line 99 cope with a NULL holder?
> 
> No anonymous class's holders are the mirror which cannot be null.  I can add this before line 99:
> 
>     assert(holder != NULL, "holder of anonymous class is the mirror which is never null");
> 
>> 
>> Basically I'm questioning whether the ik->klass_holder() on line 91
>> can ever legitimately return NULL.
> 
> Yes it can and does.

I see; I was confused by some potential future changes that we've
discussed.  There may need to be further changes in this area later,
but those changes are likely made easier by this one, and in the
meantime this looks good.