RFR 8215155: Remove get_insert() from concurrent hashtable and gtests

[Robbin Ehn]

On 1/10/19 4:08 AM, coleen.phillimore at oracle.com wrote:
> 
> 
> On 1/9/19 2:48 PM, Gerard Ziemski wrote:
>> hi Robbin, Coleen,
>>
>> I apologize, but in my haste to try to push this issue before I went off for 
>> holiday break (which I failed), I missed another issue, so I’d like to ask you 
>> to review this one last small change from webrev2:
>>
>> line 129 in jdk/test/hotspot/gtest/utilities/test_concurrentHashtable.cpp 
>> should be
>>
>> 129     cht_insert_helper(thr, cht, val);
>>
>> instead of
>>
>> 129     cht_get_helper(thr, cht, val);
> 
> Makes sense.  This looks good.  You can run the gtests locally on your machine, 
> which is a lot faster.

+1

/Robbin

> 
> Coleen
>>
>> bug id:  https://bugs.openjdk.java.net/browse/JDK-8215155
>> webrev:  http://cr.openjdk.java.net/~gziemski/8215155_rev3
>> testing: Passed Mach hs_tier1,2,3
>>
>>
>> thank you
>>
>>> On Dec 13, 2018, at 9:48 AM, Gerard Ziemski <gerard.ziemski at oracle.com> wrote:
>>>
>>>> On Dec 13, 2018, at 8:58 AM, Gerard Ziemski <gerard.ziemski at oracle.com> wrote:
>>>>
>>>>
>>>>> On Dec 12, 2018, at 6:30 PM, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>>
>>>>> This looks good to me.
>>>>>
>>>>> On 12/12/18 3:01 PM, Gerard Ziemski wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Please review this enhancement followup to JDK-8214310 and JDK-8213791
>>>>>>
>>>>>> Here we redo the relevant gtest tests to use get(),insert() and are now 
>>>>>> free to remove the get_insert() API from the concurrent hash table 
>>>>>> implementation.
>>>>>>
>>>>>> bug id:  https://bugs.openjdk.java.net/browse/JDK-8215155
>>>>>> webrev:  http://cr.openjdk.java.net/~gziemski/8215155_rev1
>>>>>> Testing: Passed Mach hs_tier1, another Mach hs_tier1,2,3,4,5,6 in progress…
>>>>>>
>>>>>> P.S. I’d like to push this into JDK12 if there is no contention about this 
>>>>>> fix…
>>>>> It seems pretty straightforward.  I'm fine with your pushing this to 12 but 
>>>>> the deadline is fast approaching and I think you should have another reviewer.
>>>> Thank you for the review Coleen!
>>>>
>>>> As this is a test related change plus trivial removal of unused API, 
>>>> normally I’d mark it as trivial, but I agree with you - a second reviewer 
>>>> here would be most appreciated.
>>> I just discovered that I had an assert reversed in my webrev1 fix - in the 
>>> file jdk/test/hotspot/gtest/utilities/test_concurrentHashtable.cpp, line 108:
>>>
>>> EXPECT_EQ(cht->insert(thr, stl, val), false) << "Inserting an unique value 
>>> failed.”;
>>>
>>> should be:
>>>
>>> EXPECT_EQ(cht->insert(thr, stl, val), true) << "Inserting an unique value 
>>> failed.”;
>>>
>>> I fixed it and posted http://cr.openjdk.java.net/~gziemski/8215155_rev2
>>>
>>> I fired another Mach5 test (why wasn’t it caught?), but now I feel a bit less 
>>> confident about pushing it into jdk12…
>>>
>>>
>>> cheers
>