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

classic Classic list List threaded Threaded
6 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
>