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

[Erik Österlund]

Hi Coleen,

Thanks for the review.

/Erik

On 2018-01-08 14:26, coleen.phillimore at oracle.com 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, 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.
>>
>
> 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
>>>>
>>>
>>
>