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

Erik Österlund erik.osterlund at oracle.com
Mon Jan 8 14:39:28 UTC 2018


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, coleen.phillimore at oracle.com 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
>>>>
>>>
>>



More information about the hotspot-runtime-dev mailing list