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

[Erik Österlund]

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