[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR (S): 8192003: Refactor weak references in StringTable to use the Access API
From:       coleen.phillimore () oracle ! com
Date:       2017-12-12 22:14:01
Message-ID: b398262d-bdbc-958f-c615-c7e5eb6967be () oracle ! com
[Download RAW message or body]


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?

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?

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
>

[prev in list] [next in list] [prev in thread] [next in thread]