RFR 8214310: SymbolTable: Use get and insert

[Gerard Ziemski]

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.


> 
> 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


> 
> 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
>>> 
>