RFR 8214310: SymbolTable: Use get and insert

[coleen.phillimore at oracle.com]

On 12/10/18 2:34 PM, Gerard Ziemski wrote:
> hi Coleen,
>
> Thank you for the review.
>
>> On Dec 10, 2018, at 12:56 PM, coleen.phillimore at oracle.com wrote:
>>
>>
>>
>> On 12/10/18 1:31 PM, Claes Redestad wrote:
>>> Hi Gerard,
>>>
>>> +    if (value->refcount() != PERM_REFCOUNT) {
>>> +      if (value->refcount() != 0) {
>>> +        assert(value->refcount() == 1, "expected newly created symbol");
>>> +        value->decrement_refcount();
>>> +      }
>>> +      assert(value->refcount() == 0, "expected dead symbol");
>>> +    }
>>>
>>> I think this could be simplified:
>>>
>>> +    if (value->refcount() == 1) {
>>> +      value->decrement_refcount();
>>> +      assert(value->refcount() == 0, "expected dead symbol");
>>> +    }
>>>
>>> -- 
>>>
>>> In do_add_if_needed, could &rehash_warning be set to true by one call
>>> and then reset to false by a subsequent call? Would it be more robust to
>>> call update_needs_hash inside the loop and rearrange accordingly?
>> The needs_rehashing is never turned back to false, so I think it's fine outside the loop.  To be more consistent with StringTable, maybe it should go into the loop though.
> I did consider this, but I’d rather keep this as is and change the StringTable later to match the SymbolTable. I don’t think there is a real need to test that hint on every loop, but if there was one, then even the StringTable is not correct either - we would have to do it for get(),insert() when they return “false” as well, not just when they return “true”, as it is right now.

Okay.
>
>
>> This change looks really good to me.  Should you remove get_insert_lazy with this change and internal_get_insert with this change?
>>
>> Or file a different RFE.  There are probably tests for this API.
> Filed enhancement https://bugs.openjdk.java.net/browse/JDK-8215155
Great.  rev2 webrev looks good.

Thanks!
Coleen

>
>
>> thanks,
>> Coleen
>>> Thanks!
>>>
>>> /Claes
>>>
>>> On 2018-12-10 18:14, Gerard Ziemski wrote:
>>>> Hi all,
>>>>
>>>> Please review this fix, which is similar to 8213791, but here we implement it for the SymbolTable, whereas before we did it for the StringTable.
>>>>
>>>> I’d like to see this go in JDK12, but if we determine that there is any part of it that is too controversial, I’m OK with waiting for JDK13
>>>>
>>>> References:
>>>>
>>>> bug id: https://bugs.openjdk.java.net/browse/JDK-8214310
>>>> webrev: http://cr.openjdk.java.net/~gziemski/8214310_rev1
>>>> tests:  passes Mach5 hs_tier1,2,3,4,5
>>>>
>>>>
>>>> Cheers
>>>>