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

[Erik Österlund]

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