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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
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
Reply | Threaded
Open this post in threaded view
|

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

Erik Osterlund
Hi Coleen,

Looks fantastic.

Thanks,
/Erik

On 2017-11-08 01:38, [hidden email] 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

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
Thank you Erik!
Coleen

On 11/8/17 11:26 AM, Erik Österlund wrote:

> Hi Coleen,
>
> Looks fantastic.
>
> Thanks,
> /Erik
>
> On 2017-11-08 01:38, [hidden email] 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
>

Reply | Threaded
Open this post in threaded view
|

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

Kim Barrett
In reply to this post by coleen.phillimore
> On Nov 7, 2017, at 7:38 PM, [hidden email] 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

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.

And can the call to get_object at line 99 cope with a NULL holder?

Basically I'm questioning whether the ik->klass_holder() on line 91
can ever legitimately return NULL.

------------------------------------------------------------------------------

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore

Kim,

Thank you for revewing.

On 11/8/17 2:28 PM, Kim Barrett wrote:

>> On Nov 7, 2017, at 7:38 PM, [hidden email] 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.

Thanks,
Coleen



>
> ------------------------------------------------------------------------------
>

Reply | Threaded
Open this post in threaded view
|

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

Kim Barrett
> On Nov 8, 2017, at 2:58 PM, [hidden email] wrote:
>
>
> Kim,
>
> Thank you for revewing.
>
> On 11/8/17 2:28 PM, Kim Barrett wrote:
>>> On Nov 7, 2017, at 7:38 PM, [hidden email] 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.

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
Thanks Kim!
Coleen

On 11/13/17 11:24 PM, Kim Barrett wrote:

>> On Nov 8, 2017, at 2:58 PM, [hidden email] wrote:
>>
>>
>> Kim,
>>
>> Thank you for revewing.
>>
>> On 11/8/17 2:28 PM, Kim Barrett wrote:
>>>> On Nov 7, 2017, at 7:38 PM, [hidden email] 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.
>