RFR 8215155: Remove get_insert() from concurrent hashtable and gtests
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Dec 17 21:44:17 UTC 2018
On 12/13/18 10:48 AM, Gerard Ziemski 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…
I can't explain that either. You can run locally via *make
test-hotspot-gtest
*and with some more options to just run the test.
I read it a bit more carefully this time. It looks good.
Coleen
**
>
> cheers
>
>
>
More information about the hotspot-runtime-dev
mailing list