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

[Per Lidén]

Looks good!

/Per

> On 30 Nov 2017, at 14:44, Erik Österlund <erik.osterlund at oracle.com> 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
>