RFR (S): 8192003: Refactor weak references in StringTable to use the Access API

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

RFR (S): 8192003: Refactor weak references in StringTable to use the Access API

Erik Österlund-2
Hi,

The StringTable contains weak references to oops. Today the weak
semantics is managed using explicit calls to G1 SATB enqueue barriers.

Now that the Access API is available, it should be used instead as it is
more modular.

This change fixes that by making these oops ON_PHANTOM_OOP_REF with a
corresponding AS_NO_KEEPALIVE accessor when we do not want to keep it
alive, much like my previous changes to other weak tables.

Webrev:
http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00/

Bug:
https://bugs.openjdk.java.net/browse/JDK-8192003

Thanks,
/Erik
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8192003: Refactor weak references in StringTable to use the Access API

Per Liden
Hi Erik,

On 2017-11-28 17:50, Erik Österlund wrote:

> Hi,
>
> The StringTable contains weak references to oops. Today the weak
> semantics is managed using explicit calls to G1 SATB enqueue barriers.
>
> Now that the Access API is available, it should be used instead as it is
> more modular.
>
> This change fixes that by making these oops ON_PHANTOM_OOP_REF with a
> corresponding AS_NO_KEEPALIVE accessor when we do not want to keep it
> alive, much like my previous changes to other weak tables.
>
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00/

share/classfile/javaClasses.inline.hpp
--------------------------------------

   57 typeArrayOop java_lang_String::value_no_keepalive(oop java_string) {
   58   assert(initialized && (value_offset > 0), "Must be initialized");
   59   assert(is_instance(java_string), "must be java_string");
   60   oop value =
HeapAccess<AS_NO_KEEPALIVE>::oop_load_at(java_string, value_offset);
   61   return (typeArrayOop)value;
   62 }

How about pushing this barrier down to oopDesc, with a
oopDesc::obj_field_special_access<DecoratorSet D>(...) function?


   76   typeArrayOop value =
java_lang_String::value_no_keepalive(java_string);
   77   assert(initialized, "Must be initialized");
   78   assert(is_instance(java_string), "must be java_string");

Looks like you accidentally moved the value_no_keepalive() call above
the asserts?


share/classfile/stringTable.cpp
-------------------------------

  155       oop string = string_object_no_keepalive(l);
  156       if (java_lang_String::equals(string, name, len)) {
  157         return string_object(l);
  158       }

Can we please add a comment here, saying that returning "string" would
be very bad, or maybe even fold this a bit, so that no one will be
tempted to ever return that value, something like:

if (java_lang_String::equals(string_object_no_keepalive(l), name, len)) {
     // Comment saying why we must call string_object() here...
     return string_object(l);
}

cheers,
Per

>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8192003
>
> Thanks,
> /Erik
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8192003: Refactor weak references in StringTable to use the Access API

Erik Österlund-2
Hi Per,

Thank you for reviewing this.

New full webrev:
http://cr.openjdk.java.net/~eosterlund/8192003/webrev.01/

New incremental webrev:
http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00_01/

On 2017-11-30 11:32, Per Liden wrote:

> Hi Erik,
>
> On 2017-11-28 17:50, Erik Österlund wrote:
>> Hi,
>>
>> The StringTable contains weak references to oops. Today the weak
>> semantics is managed using explicit calls to G1 SATB enqueue barriers.
>>
>> Now that the Access API is available, it should be used instead as it is
>> more modular.
>>
>> This change fixes that by making these oops ON_PHANTOM_OOP_REF with a
>> corresponding AS_NO_KEEPALIVE accessor when we do not want to keep it
>> alive, much like my previous changes to other weak tables.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00/
>
> share/classfile/javaClasses.inline.hpp
> --------------------------------------
>
>   57 typeArrayOop java_lang_String::value_no_keepalive(oop java_string) {
>   58   assert(initialized && (value_offset > 0), "Must be initialized");
>   59   assert(is_instance(java_string), "must be java_string");
>   60   oop value =
> HeapAccess<AS_NO_KEEPALIVE>::oop_load_at(java_string, value_offset);
>   61   return (typeArrayOop)value;
>   62 }
>
> How about pushing this barrier down to oopDesc, with a
> oopDesc::obj_field_special_access<DecoratorSet D>(...) function?

Sounds doable. Fixed. (Although I called the method just
"obj_field_special", hope nobody minds...)

>
>   76   typeArrayOop value =
> java_lang_String::value_no_keepalive(java_string);
>   77   assert(initialized, "Must be initialized");
>   78   assert(is_instance(java_string), "must be java_string");
>
> Looks like you accidentally moved the value_no_keepalive() call above
> the asserts?

Fixed.

>
> share/classfile/stringTable.cpp
> -------------------------------
>
>  155       oop string = string_object_no_keepalive(l);
>  156       if (java_lang_String::equals(string, name, len)) {
>  157         return string_object(l);
>  158       }
>
> Can we please add a comment here, saying that returning "string" would
> be very bad, or maybe even fold this a bit, so that no one will be
> tempted to ever return that value, something like:
>
> if (java_lang_String::equals(string_object_no_keepalive(l), name, len)) {
>     // Comment saying why we must call string_object() here...
>     return string_object(l);
> }

Fixed.

Thanks,
/Erik

> cheers,
> Per
>
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8192003
>>
>> Thanks,
>> /Erik

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8192003: Refactor weak references in StringTable to use the Access API

Per Liden
Looks good!

/Per

> On 30 Nov 2017, at 14:44, Erik Österlund <[hidden email]> wrote:
>
> Hi Per,
>
> Thank you for reviewing this.
>
> New full webrev:
> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.01/
>
> New incremental webrev:
> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00_01/
>
>> On 2017-11-30 11:32, Per Liden wrote:
>> Hi Erik,
>>
>>> On 2017-11-28 17:50, Erik Österlund wrote:
>>> Hi,
>>>
>>> The StringTable contains weak references to oops. Today the weak
>>> semantics is managed using explicit calls to G1 SATB enqueue barriers.
>>>
>>> Now that the Access API is available, it should be used instead as it is
>>> more modular.
>>>
>>> This change fixes that by making these oops ON_PHANTOM_OOP_REF with a
>>> corresponding AS_NO_KEEPALIVE accessor when we do not want to keep it
>>> alive, much like my previous changes to other weak tables.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00/
>>
>> share/classfile/javaClasses.inline.hpp
>> --------------------------------------
>>
>>  57 typeArrayOop java_lang_String::value_no_keepalive(oop java_string) {
>>  58   assert(initialized && (value_offset > 0), "Must be initialized");
>>  59   assert(is_instance(java_string), "must be java_string");
>>  60   oop value = HeapAccess<AS_NO_KEEPALIVE>::oop_load_at(java_string, value_offset);
>>  61   return (typeArrayOop)value;
>>  62 }
>>
>> How about pushing this barrier down to oopDesc, with a oopDesc::obj_field_special_access<DecoratorSet D>(...) function?
>
> Sounds doable. Fixed. (Although I called the method just "obj_field_special", hope nobody minds...)
>
>>
>>  76   typeArrayOop value = java_lang_String::value_no_keepalive(java_string);
>>  77   assert(initialized, "Must be initialized");
>>  78   assert(is_instance(java_string), "must be java_string");
>>
>> Looks like you accidentally moved the value_no_keepalive() call above the asserts?
>
> Fixed.
>
>>
>> share/classfile/stringTable.cpp
>> -------------------------------
>>
>> 155       oop string = string_object_no_keepalive(l);
>> 156       if (java_lang_String::equals(string, name, len)) {
>> 157         return string_object(l);
>> 158       }
>>
>> Can we please add a comment here, saying that returning "string" would be very bad, or maybe even fold this a bit, so that no one will be tempted to ever return that value, something like:
>>
>> if (java_lang_String::equals(string_object_no_keepalive(l), name, len)) {
>>    // Comment saying why we must call string_object() here...
>>    return string_object(l);
>> }
>
> Fixed.
>
> Thanks,
> /Erik
>
>> cheers,
>> Per
>>
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8192003
>>>
>>> Thanks,
>>> /Erik
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8192003: Refactor weak references in StringTable to use the Access API

Erik Österlund-2
Hi Per,

Thanks for the review.

/Erik

> On 30 Nov 2017, at 16:43, Per Lidén <[hidden email]> wrote:
>
> Looks good!
>
> /Per
>
>> On 30 Nov 2017, at 14:44, Erik Österlund <[hidden email]> wrote:
>>
>> Hi Per,
>>
>> Thank you for reviewing this.
>>
>> New full webrev:
>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.01/
>>
>> New incremental webrev:
>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00_01/
>>
>>> On 2017-11-30 11:32, Per Liden wrote:
>>> Hi Erik,
>>>
>>>> On 2017-11-28 17:50, Erik Österlund wrote:
>>>> Hi,
>>>>
>>>> The StringTable contains weak references to oops. Today the weak
>>>> semantics is managed using explicit calls to G1 SATB enqueue barriers.
>>>>
>>>> Now that the Access API is available, it should be used instead as it is
>>>> more modular.
>>>>
>>>> This change fixes that by making these oops ON_PHANTOM_OOP_REF with a
>>>> corresponding AS_NO_KEEPALIVE accessor when we do not want to keep it
>>>> alive, much like my previous changes to other weak tables.
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00/
>>>
>>> share/classfile/javaClasses.inline.hpp
>>> --------------------------------------
>>>
>>> 57 typeArrayOop java_lang_String::value_no_keepalive(oop java_string) {
>>> 58   assert(initialized && (value_offset > 0), "Must be initialized");
>>> 59   assert(is_instance(java_string), "must be java_string");
>>> 60   oop value = HeapAccess<AS_NO_KEEPALIVE>::oop_load_at(java_string, value_offset);
>>> 61   return (typeArrayOop)value;
>>> 62 }
>>>
>>> How about pushing this barrier down to oopDesc, with a oopDesc::obj_field_special_access<DecoratorSet D>(...) function?
>>
>> Sounds doable. Fixed. (Although I called the method just "obj_field_special", hope nobody minds...)
>>
>>>
>>> 76   typeArrayOop value = java_lang_String::value_no_keepalive(java_string);
>>> 77   assert(initialized, "Must be initialized");
>>> 78   assert(is_instance(java_string), "must be java_string");
>>>
>>> Looks like you accidentally moved the value_no_keepalive() call above the asserts?
>>
>> Fixed.
>>
>>>
>>> share/classfile/stringTable.cpp
>>> -------------------------------
>>>
>>> 155       oop string = string_object_no_keepalive(l);
>>> 156       if (java_lang_String::equals(string, name, len)) {
>>> 157         return string_object(l);
>>> 158       }
>>>
>>> Can we please add a comment here, saying that returning "string" would be very bad, or maybe even fold this a bit, so that no one will be tempted to ever return that value, something like:
>>>
>>> if (java_lang_String::equals(string_object_no_keepalive(l), name, len)) {
>>>   // Comment saying why we must call string_object() here...
>>>   return string_object(l);
>>> }
>>
>> Fixed.
>>
>> Thanks,
>> /Erik
>>
>>> cheers,
>>> Per
>>>
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8192003
>>>>
>>>> Thanks,
>>>> /Erik
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8192003: Refactor weak references in StringTable to use the Access API

coleen.phillimore
In reply to this post by Erik Österlund-2

Hi Erik,

This looks great.  Would obj_field_access<>() be a better name than
obj_field_special<> since it's the access which is so special?

Like in the other tables, it would be nice to disallow the literal()
call for Hashtable<oop>s so we don't mistakenly add one.  Can you
declare a ShouldNotReachHere() literal function for these?  Or version
with no body that would cause a linktime error?

Thanks,
Coleen


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

> Hi Per,
>
> Thank you for reviewing this.
>
> New full webrev:
> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.01/
>
> New incremental webrev:
> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00_01/
>
> On 2017-11-30 11:32, Per Liden wrote:
>> Hi Erik,
>>
>> On 2017-11-28 17:50, Erik Österlund wrote:
>>> Hi,
>>>
>>> The StringTable contains weak references to oops. Today the weak
>>> semantics is managed using explicit calls to G1 SATB enqueue barriers.
>>>
>>> Now that the Access API is available, it should be used instead as
>>> it is
>>> more modular.
>>>
>>> This change fixes that by making these oops ON_PHANTOM_OOP_REF with a
>>> corresponding AS_NO_KEEPALIVE accessor when we do not want to keep it
>>> alive, much like my previous changes to other weak tables.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00/
>>
>> share/classfile/javaClasses.inline.hpp
>> --------------------------------------
>>
>>   57 typeArrayOop java_lang_String::value_no_keepalive(oop
>> java_string) {
>>   58   assert(initialized && (value_offset > 0), "Must be initialized");
>>   59   assert(is_instance(java_string), "must be java_string");
>>   60   oop value =
>> HeapAccess<AS_NO_KEEPALIVE>::oop_load_at(java_string, value_offset);
>>   61   return (typeArrayOop)value;
>>   62 }
>>
>> How about pushing this barrier down to oopDesc, with a
>> oopDesc::obj_field_special_access<DecoratorSet D>(...) function?
>
> Sounds doable. Fixed. (Although I called the method just
> "obj_field_special", hope nobody minds...)
>
>>
>>   76   typeArrayOop value =
>> java_lang_String::value_no_keepalive(java_string);
>>   77   assert(initialized, "Must be initialized");
>>   78   assert(is_instance(java_string), "must be java_string");
>>
>> Looks like you accidentally moved the value_no_keepalive() call above
>> the asserts?
>
> Fixed.
>
>>
>> share/classfile/stringTable.cpp
>> -------------------------------
>>
>>  155       oop string = string_object_no_keepalive(l);
>>  156       if (java_lang_String::equals(string, name, len)) {
>>  157         return string_object(l);
>>  158       }
>>
>> Can we please add a comment here, saying that returning "string"
>> would be very bad, or maybe even fold this a bit, so that no one will
>> be tempted to ever return that value, something like:
>>
>> if (java_lang_String::equals(string_object_no_keepalive(l), name,
>> len)) {
>>     // Comment saying why we must call string_object() here...
>>     return string_object(l);
>> }
>
> Fixed.
>
> Thanks,
> /Erik
>
>> cheers,
>> Per
>>
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8192003
>>>
>>> Thanks,
>>> /Erik
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8192003: Refactor weak references in StringTable to use the Access API

Erik Österlund-2
Hi Coleen,

Thank you for the review.

On 2017-12-12 23:14, [hidden email] wrote:
>
> Hi Erik,
>
> This looks great.  Would obj_field_access<>() be a better name than
> obj_field_special<> since it's the access which is so special?

Sure, that sounds good to me.

Incremental webrev:
cr.openjdk.java.net/~eosterlund/8192003/webrev.01_02/

Full webrev:
cr.openjdk.java.net/~eosterlund/8192003/webrev.02/

> Like in the other tables, it would be nice to disallow the literal()
> call for Hashtable<oop>s so we don't mistakenly add one.  Can you
> declare a ShouldNotReachHere() literal function for these?  Or version
> with no body that would cause a linktime error?

Yes, but perhaps that should be done as a follow up RFE after these
tables have been purged from evil.

Thanks,
/Erik

> Thanks,
> Coleen
>
>
> On 11/30/17 8:44 AM, Erik Österlund wrote:
>> Hi Per,
>>
>> Thank you for reviewing this.
>>
>> New full webrev:
>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.01/
>>
>> New incremental webrev:
>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00_01/
>>
>> On 2017-11-30 11:32, Per Liden wrote:
>>> Hi Erik,
>>>
>>> On 2017-11-28 17:50, Erik Österlund wrote:
>>>> Hi,
>>>>
>>>> The StringTable contains weak references to oops. Today the weak
>>>> semantics is managed using explicit calls to G1 SATB enqueue barriers.
>>>>
>>>> Now that the Access API is available, it should be used instead as
>>>> it is
>>>> more modular.
>>>>
>>>> This change fixes that by making these oops ON_PHANTOM_OOP_REF with a
>>>> corresponding AS_NO_KEEPALIVE accessor when we do not want to keep it
>>>> alive, much like my previous changes to other weak tables.
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00/
>>>
>>> share/classfile/javaClasses.inline.hpp
>>> --------------------------------------
>>>
>>>   57 typeArrayOop java_lang_String::value_no_keepalive(oop
>>> java_string) {
>>>   58   assert(initialized && (value_offset > 0), "Must be
>>> initialized");
>>>   59   assert(is_instance(java_string), "must be java_string");
>>>   60   oop value =
>>> HeapAccess<AS_NO_KEEPALIVE>::oop_load_at(java_string, value_offset);
>>>   61   return (typeArrayOop)value;
>>>   62 }
>>>
>>> How about pushing this barrier down to oopDesc, with a
>>> oopDesc::obj_field_special_access<DecoratorSet D>(...) function?
>>
>> Sounds doable. Fixed. (Although I called the method just
>> "obj_field_special", hope nobody minds...)
>>
>>>
>>>   76   typeArrayOop value =
>>> java_lang_String::value_no_keepalive(java_string);
>>>   77   assert(initialized, "Must be initialized");
>>>   78   assert(is_instance(java_string), "must be java_string");
>>>
>>> Looks like you accidentally moved the value_no_keepalive() call
>>> above the asserts?
>>
>> Fixed.
>>
>>>
>>> share/classfile/stringTable.cpp
>>> -------------------------------
>>>
>>>  155       oop string = string_object_no_keepalive(l);
>>>  156       if (java_lang_String::equals(string, name, len)) {
>>>  157         return string_object(l);
>>>  158       }
>>>
>>> Can we please add a comment here, saying that returning "string"
>>> would be very bad, or maybe even fold this a bit, so that no one
>>> will be tempted to ever return that value, something like:
>>>
>>> if (java_lang_String::equals(string_object_no_keepalive(l), name,
>>> len)) {
>>>     // Comment saying why we must call string_object() here...
>>>     return string_object(l);
>>> }
>>
>> Fixed.
>>
>> Thanks,
>> /Erik
>>
>>> cheers,
>>> Per
>>>
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8192003
>>>>
>>>> Thanks,
>>>> /Erik
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8192003: Refactor weak references in StringTable to use the Access API

coleen.phillimore

Hi Erik,

The renaming looks good.

On 1/8/18 6:04 AM, Erik Österlund wrote:

> Hi Coleen,
>
> Thank you for the review.
>
> On 2017-12-12 23:14, [hidden email] wrote:
>>
>> Hi Erik,
>>
>> This looks great.  Would obj_field_access<>() be a better name than
>> obj_field_special<> since it's the access which is so special?
>
> Sure, that sounds good to me.
>
> Incremental webrev:
> cr.openjdk.java.net/~eosterlund/8192003/webrev.01_02/
>
> Full webrev:
> cr.openjdk.java.net/~eosterlund/8192003/webrev.02/
>
>> Like in the other tables, it would be nice to disallow the literal()
>> call for Hashtable<oop>s so we don't mistakenly add one.  Can you
>> declare a ShouldNotReachHere() literal function for these?  Or
>> version with no body that would cause a linktime error?
>
> Yes, but perhaps that should be done as a follow up RFE after these
> tables have been purged from evil.
>

Yes, not now.  We are working on a concurrent hashtable for these
anyway, so this problem is likely to go away.

thanks,
Coleen

> Thanks,
> /Erik
>
>> Thanks,
>> Coleen
>>
>>
>> On 11/30/17 8:44 AM, Erik Österlund wrote:
>>> Hi Per,
>>>
>>> Thank you for reviewing this.
>>>
>>> New full webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.01/
>>>
>>> New incremental webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00_01/
>>>
>>> On 2017-11-30 11:32, Per Liden wrote:
>>>> Hi Erik,
>>>>
>>>> On 2017-11-28 17:50, Erik Österlund wrote:
>>>>> Hi,
>>>>>
>>>>> The StringTable contains weak references to oops. Today the weak
>>>>> semantics is managed using explicit calls to G1 SATB enqueue
>>>>> barriers.
>>>>>
>>>>> Now that the Access API is available, it should be used instead as
>>>>> it is
>>>>> more modular.
>>>>>
>>>>> This change fixes that by making these oops ON_PHANTOM_OOP_REF with a
>>>>> corresponding AS_NO_KEEPALIVE accessor when we do not want to keep it
>>>>> alive, much like my previous changes to other weak tables.
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00/
>>>>
>>>> share/classfile/javaClasses.inline.hpp
>>>> --------------------------------------
>>>>
>>>>   57 typeArrayOop java_lang_String::value_no_keepalive(oop
>>>> java_string) {
>>>>   58   assert(initialized && (value_offset > 0), "Must be
>>>> initialized");
>>>>   59   assert(is_instance(java_string), "must be java_string");
>>>>   60   oop value =
>>>> HeapAccess<AS_NO_KEEPALIVE>::oop_load_at(java_string, value_offset);
>>>>   61   return (typeArrayOop)value;
>>>>   62 }
>>>>
>>>> How about pushing this barrier down to oopDesc, with a
>>>> oopDesc::obj_field_special_access<DecoratorSet D>(...) function?
>>>
>>> Sounds doable. Fixed. (Although I called the method just
>>> "obj_field_special", hope nobody minds...)
>>>
>>>>
>>>>   76   typeArrayOop value =
>>>> java_lang_String::value_no_keepalive(java_string);
>>>>   77   assert(initialized, "Must be initialized");
>>>>   78   assert(is_instance(java_string), "must be java_string");
>>>>
>>>> Looks like you accidentally moved the value_no_keepalive() call
>>>> above the asserts?
>>>
>>> Fixed.
>>>
>>>>
>>>> share/classfile/stringTable.cpp
>>>> -------------------------------
>>>>
>>>>  155       oop string = string_object_no_keepalive(l);
>>>>  156       if (java_lang_String::equals(string, name, len)) {
>>>>  157         return string_object(l);
>>>>  158       }
>>>>
>>>> Can we please add a comment here, saying that returning "string"
>>>> would be very bad, or maybe even fold this a bit, so that no one
>>>> will be tempted to ever return that value, something like:
>>>>
>>>> if (java_lang_String::equals(string_object_no_keepalive(l), name,
>>>> len)) {
>>>>     // Comment saying why we must call string_object() here...
>>>>     return string_object(l);
>>>> }
>>>
>>> Fixed.
>>>
>>> Thanks,
>>> /Erik
>>>
>>>> cheers,
>>>> Per
>>>>
>>>>>
>>>>> Bug:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8192003
>>>>>
>>>>> Thanks,
>>>>> /Erik
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8192003: Refactor weak references in StringTable to use the Access API

Erik Österlund-2
Hi Coleen,

Thanks for the review.

/Erik

On 2018-01-08 14:26, [hidden email] wrote:

>
> Hi Erik,
>
> The renaming looks good.
>
> On 1/8/18 6:04 AM, Erik Österlund wrote:
>> Hi Coleen,
>>
>> Thank you for the review.
>>
>> On 2017-12-12 23:14, [hidden email] wrote:
>>>
>>> Hi Erik,
>>>
>>> This looks great.  Would obj_field_access<>() be a better name than
>>> obj_field_special<> since it's the access which is so special?
>>
>> Sure, that sounds good to me.
>>
>> Incremental webrev:
>> cr.openjdk.java.net/~eosterlund/8192003/webrev.01_02/
>>
>> Full webrev:
>> cr.openjdk.java.net/~eosterlund/8192003/webrev.02/
>>
>>> Like in the other tables, it would be nice to disallow the literal()
>>> call for Hashtable<oop>s so we don't mistakenly add one.  Can you
>>> declare a ShouldNotReachHere() literal function for these?  Or
>>> version with no body that would cause a linktime error?
>>
>> Yes, but perhaps that should be done as a follow up RFE after these
>> tables have been purged from evil.
>>
>
> Yes, not now.  We are working on a concurrent hashtable for these
> anyway, so this problem is likely to go away.
>
> thanks,
> Coleen
>> Thanks,
>> /Erik
>>
>>> Thanks,
>>> Coleen
>>>
>>>
>>> On 11/30/17 8:44 AM, Erik Österlund wrote:
>>>> Hi Per,
>>>>
>>>> Thank you for reviewing this.
>>>>
>>>> New full webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.01/
>>>>
>>>> New incremental webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00_01/
>>>>
>>>> On 2017-11-30 11:32, Per Liden wrote:
>>>>> Hi Erik,
>>>>>
>>>>> On 2017-11-28 17:50, Erik Österlund wrote:
>>>>>> Hi,
>>>>>>
>>>>>> The StringTable contains weak references to oops. Today the weak
>>>>>> semantics is managed using explicit calls to G1 SATB enqueue
>>>>>> barriers.
>>>>>>
>>>>>> Now that the Access API is available, it should be used instead
>>>>>> as it is
>>>>>> more modular.
>>>>>>
>>>>>> This change fixes that by making these oops ON_PHANTOM_OOP_REF
>>>>>> with a
>>>>>> corresponding AS_NO_KEEPALIVE accessor when we do not want to
>>>>>> keep it
>>>>>> alive, much like my previous changes to other weak tables.
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00/
>>>>>
>>>>> share/classfile/javaClasses.inline.hpp
>>>>> --------------------------------------
>>>>>
>>>>>   57 typeArrayOop java_lang_String::value_no_keepalive(oop
>>>>> java_string) {
>>>>>   58   assert(initialized && (value_offset > 0), "Must be
>>>>> initialized");
>>>>>   59   assert(is_instance(java_string), "must be java_string");
>>>>>   60   oop value =
>>>>> HeapAccess<AS_NO_KEEPALIVE>::oop_load_at(java_string, value_offset);
>>>>>   61   return (typeArrayOop)value;
>>>>>   62 }
>>>>>
>>>>> How about pushing this barrier down to oopDesc, with a
>>>>> oopDesc::obj_field_special_access<DecoratorSet D>(...) function?
>>>>
>>>> Sounds doable. Fixed. (Although I called the method just
>>>> "obj_field_special", hope nobody minds...)
>>>>
>>>>>
>>>>>   76   typeArrayOop value =
>>>>> java_lang_String::value_no_keepalive(java_string);
>>>>>   77   assert(initialized, "Must be initialized");
>>>>>   78   assert(is_instance(java_string), "must be java_string");
>>>>>
>>>>> Looks like you accidentally moved the value_no_keepalive() call
>>>>> above the asserts?
>>>>
>>>> Fixed.
>>>>
>>>>>
>>>>> share/classfile/stringTable.cpp
>>>>> -------------------------------
>>>>>
>>>>>  155       oop string = string_object_no_keepalive(l);
>>>>>  156       if (java_lang_String::equals(string, name, len)) {
>>>>>  157         return string_object(l);
>>>>>  158       }
>>>>>
>>>>> Can we please add a comment here, saying that returning "string"
>>>>> would be very bad, or maybe even fold this a bit, so that no one
>>>>> will be tempted to ever return that value, something like:
>>>>>
>>>>> if (java_lang_String::equals(string_object_no_keepalive(l), name,
>>>>> len)) {
>>>>>     // Comment saying why we must call string_object() here...
>>>>>     return string_object(l);
>>>>> }
>>>>
>>>> Fixed.
>>>>
>>>> Thanks,
>>>> /Erik
>>>>
>>>>> cheers,
>>>>> Per
>>>>>
>>>>>>
>>>>>> Bug:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8192003
>>>>>>
>>>>>> Thanks,
>>>>>> /Erik
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8192003: Refactor weak references in StringTable to use the Access API

Per Liden
In reply to this post by Erik Österlund-2
Still looks good!

/Per

On 2018-01-08 12:04, Erik Österlund wrote:

> Hi Coleen,
>
> Thank you for the review.
>
> On 2017-12-12 23:14, [hidden email] wrote:
>>
>> Hi Erik,
>>
>> This looks great.  Would obj_field_access<>() be a better name than
>> obj_field_special<> since it's the access which is so special?
>
> Sure, that sounds good to me.
>
> Incremental webrev:
> cr.openjdk.java.net/~eosterlund/8192003/webrev.01_02/
>
> Full webrev:
> cr.openjdk.java.net/~eosterlund/8192003/webrev.02/
>
>> Like in the other tables, it would be nice to disallow the literal()
>> call for Hashtable<oop>s so we don't mistakenly add one.  Can you
>> declare a ShouldNotReachHere() literal function for these?  Or version
>> with no body that would cause a linktime error?
>
> Yes, but perhaps that should be done as a follow up RFE after these
> tables have been purged from evil.
>
> Thanks,
> /Erik
>
>> Thanks,
>> Coleen
>>
>>
>> On 11/30/17 8:44 AM, Erik Österlund wrote:
>>> Hi Per,
>>>
>>> Thank you for reviewing this.
>>>
>>> New full webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.01/
>>>
>>> New incremental webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00_01/
>>>
>>> On 2017-11-30 11:32, Per Liden wrote:
>>>> Hi Erik,
>>>>
>>>> On 2017-11-28 17:50, Erik Österlund wrote:
>>>>> Hi,
>>>>>
>>>>> The StringTable contains weak references to oops. Today the weak
>>>>> semantics is managed using explicit calls to G1 SATB enqueue barriers.
>>>>>
>>>>> Now that the Access API is available, it should be used instead as
>>>>> it is
>>>>> more modular.
>>>>>
>>>>> This change fixes that by making these oops ON_PHANTOM_OOP_REF with a
>>>>> corresponding AS_NO_KEEPALIVE accessor when we do not want to keep it
>>>>> alive, much like my previous changes to other weak tables.
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00/
>>>>
>>>> share/classfile/javaClasses.inline.hpp
>>>> --------------------------------------
>>>>
>>>>   57 typeArrayOop java_lang_String::value_no_keepalive(oop
>>>> java_string) {
>>>>   58   assert(initialized && (value_offset > 0), "Must be
>>>> initialized");
>>>>   59   assert(is_instance(java_string), "must be java_string");
>>>>   60   oop value =
>>>> HeapAccess<AS_NO_KEEPALIVE>::oop_load_at(java_string, value_offset);
>>>>   61   return (typeArrayOop)value;
>>>>   62 }
>>>>
>>>> How about pushing this barrier down to oopDesc, with a
>>>> oopDesc::obj_field_special_access<DecoratorSet D>(...) function?
>>>
>>> Sounds doable. Fixed. (Although I called the method just
>>> "obj_field_special", hope nobody minds...)
>>>
>>>>
>>>>   76   typeArrayOop value =
>>>> java_lang_String::value_no_keepalive(java_string);
>>>>   77   assert(initialized, "Must be initialized");
>>>>   78   assert(is_instance(java_string), "must be java_string");
>>>>
>>>> Looks like you accidentally moved the value_no_keepalive() call
>>>> above the asserts?
>>>
>>> Fixed.
>>>
>>>>
>>>> share/classfile/stringTable.cpp
>>>> -------------------------------
>>>>
>>>>  155       oop string = string_object_no_keepalive(l);
>>>>  156       if (java_lang_String::equals(string, name, len)) {
>>>>  157         return string_object(l);
>>>>  158       }
>>>>
>>>> Can we please add a comment here, saying that returning "string"
>>>> would be very bad, or maybe even fold this a bit, so that no one
>>>> will be tempted to ever return that value, something like:
>>>>
>>>> if (java_lang_String::equals(string_object_no_keepalive(l), name,
>>>> len)) {
>>>>     // Comment saying why we must call string_object() here...
>>>>     return string_object(l);
>>>> }
>>>
>>> Fixed.
>>>
>>> Thanks,
>>> /Erik
>>>
>>>> cheers,
>>>> Per
>>>>
>>>>>
>>>>> Bug:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8192003
>>>>>
>>>>> Thanks,
>>>>> /Erik
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8192003: Refactor weak references in StringTable to use the Access API

Erik Österlund-2
Hi Per,

Thanks for the review!

/Erik

On 2018-01-08 15:37, Per Liden wrote:

> Still looks good!
>
> /Per
>
> On 2018-01-08 12:04, Erik Österlund wrote:
>> Hi Coleen,
>>
>> Thank you for the review.
>>
>> On 2017-12-12 23:14, [hidden email] wrote:
>>>
>>> Hi Erik,
>>>
>>> This looks great.  Would obj_field_access<>() be a better name than
>>> obj_field_special<> since it's the access which is so special?
>>
>> Sure, that sounds good to me.
>>
>> Incremental webrev:
>> cr.openjdk.java.net/~eosterlund/8192003/webrev.01_02/
>>
>> Full webrev:
>> cr.openjdk.java.net/~eosterlund/8192003/webrev.02/
>>
>>> Like in the other tables, it would be nice to disallow the literal()
>>> call for Hashtable<oop>s so we don't mistakenly add one.  Can you
>>> declare a ShouldNotReachHere() literal function for these?  Or version
>>> with no body that would cause a linktime error?
>>
>> Yes, but perhaps that should be done as a follow up RFE after these
>> tables have been purged from evil.
>>
>> Thanks,
>> /Erik
>>
>>> Thanks,
>>> Coleen
>>>
>>>
>>> On 11/30/17 8:44 AM, Erik Österlund wrote:
>>>> Hi Per,
>>>>
>>>> Thank you for reviewing this.
>>>>
>>>> New full webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.01/
>>>>
>>>> New incremental webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00_01/
>>>>
>>>> On 2017-11-30 11:32, Per Liden wrote:
>>>>> Hi Erik,
>>>>>
>>>>> On 2017-11-28 17:50, Erik Österlund wrote:
>>>>>> Hi,
>>>>>>
>>>>>> The StringTable contains weak references to oops. Today the weak
>>>>>> semantics is managed using explicit calls to G1 SATB enqueue
>>>>>> barriers.
>>>>>>
>>>>>> Now that the Access API is available, it should be used instead as
>>>>>> it is
>>>>>> more modular.
>>>>>>
>>>>>> This change fixes that by making these oops ON_PHANTOM_OOP_REF
>>>>>> with a
>>>>>> corresponding AS_NO_KEEPALIVE accessor when we do not want to
>>>>>> keep it
>>>>>> alive, much like my previous changes to other weak tables.
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~eosterlund/8192003/webrev.00/
>>>>>
>>>>> share/classfile/javaClasses.inline.hpp
>>>>> --------------------------------------
>>>>>
>>>>>   57 typeArrayOop java_lang_String::value_no_keepalive(oop
>>>>> java_string) {
>>>>>   58   assert(initialized && (value_offset > 0), "Must be
>>>>> initialized");
>>>>>   59   assert(is_instance(java_string), "must be java_string");
>>>>>   60   oop value =
>>>>> HeapAccess<AS_NO_KEEPALIVE>::oop_load_at(java_string, value_offset);
>>>>>   61   return (typeArrayOop)value;
>>>>>   62 }
>>>>>
>>>>> How about pushing this barrier down to oopDesc, with a
>>>>> oopDesc::obj_field_special_access<DecoratorSet D>(...) function?
>>>>
>>>> Sounds doable. Fixed. (Although I called the method just
>>>> "obj_field_special", hope nobody minds...)
>>>>
>>>>>
>>>>>   76   typeArrayOop value =
>>>>> java_lang_String::value_no_keepalive(java_string);
>>>>>   77   assert(initialized, "Must be initialized");
>>>>>   78   assert(is_instance(java_string), "must be java_string");
>>>>>
>>>>> Looks like you accidentally moved the value_no_keepalive() call
>>>>> above the asserts?
>>>>
>>>> Fixed.
>>>>
>>>>>
>>>>> share/classfile/stringTable.cpp
>>>>> -------------------------------
>>>>>
>>>>>  155       oop string = string_object_no_keepalive(l);
>>>>>  156       if (java_lang_String::equals(string, name, len)) {
>>>>>  157         return string_object(l);
>>>>>  158       }
>>>>>
>>>>> Can we please add a comment here, saying that returning "string"
>>>>> would be very bad, or maybe even fold this a bit, so that no one
>>>>> will be tempted to ever return that value, something like:
>>>>>
>>>>> if (java_lang_String::equals(string_object_no_keepalive(l), name,
>>>>> len)) {
>>>>>     // Comment saying why we must call string_object() here...
>>>>>     return string_object(l);
>>>>> }
>>>>
>>>> Fixed.
>>>>
>>>> Thanks,
>>>> /Erik
>>>>
>>>>> cheers,
>>>>> Per
>>>>>
>>>>>>
>>>>>> Bug:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8192003
>>>>>>
>>>>>> Thanks,
>>>>>> /Erik
>>>>
>>>
>>