JDK 10 RFR of 8176894: Provide specialized implementation for default methods putIfAbsent, computeIfAbsent, computeIfPresent, compute in TreeMap

[Sergey Kuksenko]

Hi Claes,

There is no need to backport it to 8 & 9. 8 & 9 uses default 
implementation, which is free from such tree corruption issue.


On 04/06/2017 03:45 AM, Claes Redestad wrote:
> Hi Sergey,
>
> this looks good to me*, but I can't help wonder if the modCount 
> checking is something that should be done separately as a bug fix 
> (with a higher priority) and be backported to 8 and 9? Alternatively 
> re-categorize this fix as such.
>
> Thanks!
>
> /Claes
>
> * I wouldn't mind seeing the cleanup Martin suggested, i.e., write the 
> remapValue as:
>
>     private V remapValue(Entry<K, V> t, K key, BiFunction<? super K, ? 
> super V, ? extends V> remappingFunction) {
>         V newValue = remappingFunction.apply(key, t.value);
>         if (newValue == null) {
>             deleteEntry(t);
>         } else {
>             // replace old mapping
>             t.value = newValue;
>         }
>         return newValue;
>      }
>
> On 2017-03-28 21:22, Sergey Kuksenko wrote:
>> Friendly reminder.
>>
>> Have you have chance to review the latest version?
>>
>>
>> On 03/17/2017 12:45 PM, Sergey Kuksenko wrote:
>>> Hi, All.
>>>
>>> I realized (thanks Tagir V.) that if we don't check modCount after 
>>> calling mapping function we may get corrupted tree structure.
>>> new webrev for review:
>>> http://cr.openjdk.java.net/~skuksenko/corelibs/utils/8176894/webrev.01/
>>>
>>> On 03/17/2017 11:29 AM, Martin Buchholz wrote:
>>>> Thanks!  This looks pretty good.  I have a similar effort in 
>>>> progress to improve bulk collection operations, most of which made 
>>>> it into jdk9.
>>>>
>>>> ---
>>>>
>>>> Please use standard java.util whitespace, as Aleksey suggested.
>>>>
>>>> ---
>>>>
>>>> Below (and in compute) I wpuld simply
>>>> return newValue;
>>>> saving a line of code and making it clearer that we are returning 
>>>> the result of the remappingFunction
>>>>
>>>>  676     private V remapValue(Entry<K, V> t, K key, BiFunction<? 
>>>> super K, ? super V, ? extends V> remappingFunction) {
>>>>  677         V newValue = remappingFunction.apply(key, t.value);
>>>>  678         if (newValue == null) {
>>>>  679             deleteEntry(t);
>>>>  680             return null;
>>>>  681         } else {
>>>>  682             // replace old mapping
>>>>  683             t.value = newValue;
>>>>  684             return newValue;
>>>>  685         }
>>>>  686     }
>>>> ---
>>>>
>>>> This code is surely tested but testing could also surely be 
>>>> improved.  That's probably not your job though (it may be mine!)
>>>>
>>>> I would probably try hand-injecting some bugs into a copy of the 
>>>> code and seeing if our jtreg tests catch it, to increase coverage 
>>>> confidence.
>>>>
>>>>
>>>> On Thu, Mar 16, 2017 at 12:04 PM, Sergey Kuksenko 
>>>> <sergey.kuksenko at oracle.com <mailto:sergey.kuksenko at oracle.com>> 
>>>> wrote:
>>>>
>>>>     Hi All,
>>>>
>>>>     Please, review:
>>>>     https://bugs.openjdk.java.net/browse/JDK-8176894
>>>>     <https://bugs.openjdk.java.net/browse/JDK-8176894>
>>>> http://cr.openjdk.java.net/~skuksenko/corelibs/utils/8176894/webrev.00/ 
>>>>
>>>> <http://cr.openjdk.java.net/%7Eskuksenko/corelibs/utils/8176894/webrev.00/> 
>>>>
>>>>
>>>>     The issue was created for JDK10 in order to don't disturb JDK9
>>>>     before launch.
>>>>
>>>>     --     Best regards,
>>>>     Sergey Kuksenko
>>>>
>>>>
>>>
>>
>

-- 
Best regards,
Sergey Kuksenko