RFR (S): 8192003: Refactor weak references in StringTable to use the Access API
Erik Osterlund
erik.osterlund at oracle.com
Thu Nov 30 16:17:44 UTC 2017
Hi Per,
Thanks for the review.
/Erik
> On 30 Nov 2017, at 16:43, Per Lidén <per.liden at oracle.com> wrote:
>
> 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
>>
>
More information about the hotspot-runtime-dev
mailing list