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

[coleen.phillimore at oracle.com]

Thanks Kim!
Coleen

On 11/13/17 11:24 PM, Kim Barrett wrote:
>> 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.
>