Quantcast

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

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergey Kuksenko-3
Hi All,

Please, review:
https://bugs.openjdk.java.net/browse/JDK-8176894
http://cr.openjdk.java.net/~skuksenko/corelibs/utils/8176894/webrev.00/

The issue was created for JDK10 in order to don't disturb JDK9 before
launch.

--
Best regards,
Sergey Kuksenko

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Aleksey Shipilev-4
On 03/16/2017 08:04 PM, Sergey Kuksenko wrote:
> http://cr.openjdk.java.net/~skuksenko/corelibs/utils/8176894/webrev.00/

Looks good. I assume there are enough tests to cover these paths?

*) Missing whitespace (multiple times in the similar line):

 if(replaceOld) {

Thanks,
-Aleksey

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Tagir Valeev
In reply to this post by Sergey Kuksenko-3
Hello!

Great and long-awaited feature, thanks!

I don't see that modCount is checked after calling mappingFunction in
compute/computeIfAbsent/computeIfPresent. Is it possible to break the
Map if mappingFunction changes the Map content? See similar problems
in HashMap:
https://bugs.openjdk.java.net/browse/JDK-8071667
https://bugs.openjdk.java.net/browse/JDK-8172951

With best regards,
Tagir Valeev.


SK> Hi All,

SK> Please, review:
SK> https://bugs.openjdk.java.net/browse/JDK-8176894
SK> http://cr.openjdk.java.net/~skuksenko/corelibs/utils/8176894/webrev.00/

SK> The issue was created for JDK10 in order to don't disturb JDK9 before
SK> launch.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergey Kuksenko-3

But existing (default) implementation doesn't check modCount as well. I
just preserve existing functionality.

On 03/17/2017 07:33 AM, Tagir F. Valeev wrote:

> Hello!
>
> Great and long-awaited feature, thanks!
>
> I don't see that modCount is checked after calling mappingFunction in
> compute/computeIfAbsent/computeIfPresent. Is it possible to break the
> Map if mappingFunction changes the Map content? See similar problems
> in HashMap:
> https://bugs.openjdk.java.net/browse/JDK-8071667
> https://bugs.openjdk.java.net/browse/JDK-8172951
>
> With best regards,
> Tagir Valeev.
>
>
> SK> Hi All,
>
> SK> Please, review:
> SK> https://bugs.openjdk.java.net/browse/JDK-8176894
> SK> http://cr.openjdk.java.net/~skuksenko/corelibs/utils/8176894/webrev.00/
>
> SK> The issue was created for JDK10 in order to don't disturb JDK9 before
> SK> launch.
>

--
Best regards,
Sergey Kuksenko

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Martin Buchholz-3
In reply to this post by Sergey Kuksenko-3
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 <
[hidden email]> wrote:

> Hi All,
>
> Please, review:
> https://bugs.openjdk.java.net/browse/JDK-8176894
> http://cr.openjdk.java.net/~skuksenko/corelibs/utils/8176894/webrev.00/
>
> The issue was created for JDK10 in order to don't disturb JDK9 before
> launch.
>
> --
> Best regards,
> Sergey Kuksenko
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergey Kuksenko-3
In reply to this post by Tagir Valeev
Got it. It definitely should be done.


On 03/17/2017 07:33 AM, Tagir F. Valeev wrote:

> Hello!
>
> Great and long-awaited feature, thanks!
>
> I don't see that modCount is checked after calling mappingFunction in
> compute/computeIfAbsent/computeIfPresent. Is it possible to break the
> Map if mappingFunction changes the Map content? See similar problems
> in HashMap:
> https://bugs.openjdk.java.net/browse/JDK-8071667
> https://bugs.openjdk.java.net/browse/JDK-8172951
>
> With best regards,
> Tagir Valeev.
>
>
> SK> Hi All,
>
> SK> Please, review:
> SK> https://bugs.openjdk.java.net/browse/JDK-8176894
> SK> http://cr.openjdk.java.net/~skuksenko/corelibs/utils/8176894/webrev.00/
>
> SK> The issue was created for JDK10 in order to don't disturb JDK9 before
> SK> launch.
>

--
Best regards,
Sergey Kuksenko

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergey Kuksenko-3
In reply to this post by Martin Buchholz-3
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
> <[hidden email] <mailto:[hidden email]>> 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergey Kuksenko-3
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
>> <[hidden email] <mailto:[hidden email]>> 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Claes Redestad
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
>>> <[hidden email] <mailto:[hidden email]>> 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
>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergey Kuksenko-3
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
>>>> <[hidden email] <mailto:[hidden email]>>
>>>> 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Claes Redestad
On 2017-04-10 20:43, Sergey Kuksenko wrote:
> 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.

Ah, I misread the situation then, sorry for the noise.

/Claes
Loading...