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

[coleen.phillimore at oracle.com]

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
>>>
>>
>