RFR: 8004518 & 8010122 : Default methods on Map

[Peter Levart]

On 04/11/2013 07:42 AM, Mike Duigou wrote:
> I've posted an updated webrev with the review comments I have received.
>
> http://cr.openjdk.java.net/~mduigou/JDK-8010122/1/webrev/
>
> One important point to consider:
>
> - The current implementations of compute, computeIfPresent, computeIfAbsent, merge are implemented so that they can work correctly for ConcurrentMap instances. For non-concurrent implementations it might be better to fail and throw ConcurrentModification exception whenever concurrent modification is detected. For regular Map implementations the retry behaviour will only serve to mask errors.
>
> Thoughts?
>
> Mike

Hi Mike,

Yes, that's much better. Just copy those methods to ConcurrentMap and 
replace them in Map with simplified variants. I suggest implementing 
each of them only in terms of existing (non-default) Map methods, so 
that their behaviour is stable in case any of them is selectively overriden.

For example, like the following:


     default V putIfAbsent(K key, V value) {
         V oldValue = get(key);
         if (oldValue == null && put(key, value) != null) {
             throw new ConcurrentModificationException();
         }
         return oldValue;
     }

     default V computeIfAbsent(K key, Function<? super K, ? extends V> 
mappingFunction) {
         V oldValue = get(key), newValue;
         if (oldValue == null && (newValue = mappingFunction.apply(key)) 
!= null) {
             if (put(key, newValue) != null) {
                 throw new ConcurrentModificationException();
             }
             return newValue;
         } else {
             return oldValue;
         }
     }

     default V computeIfPresent(K key, BiFunction<? super K, ? super V, 
? extends V> remappingFunction) {
         V oldValue = get(key);
         if (oldValue != null) {
             V newValue = remappingFunction.apply(key, oldValue);
             if (newValue != null) {
                 if (put(key, newValue) != oldValue) {
                     throw new ConcurrentModificationException();
                 }
             } else if (remove(key) != oldValue) {
                 throw new ConcurrentModificationException();
             }
             return newValue;
         }
         return oldValue;
     }

     default V compute(K key, BiFunction<? super K, ? super V, ? extends 
V> remappingFunction) {
         V oldValue = get(key);
         V newValue = remappingFunction.apply(key, oldValue);
         if (newValue != null) {
             if (put(key, newValue) != oldValue) {
                 throw new ConcurrentModificationException();
             }
         } else if (*oldValue != null* && remove(key) != oldValue) {
             throw new ConcurrentModificationException();
         }
         return newValue;
     }

     default V merge(K key, V value,
                     BiFunction<? super V, ? super V, ? extends V> 
remappingFunction) {
         V oldValue = get(key);
         V newValue = oldValue == null ? value : 
remappingFunction.apply(oldValue, value);
         if (newValue != null) {
             if (put(key, newValue) != oldValue) {
                 throw new ConcurrentModificationException();
             }
         } else if (*oldValue != null* && remove(key) != oldValue) {
             throw new ConcurrentModificationException();
         }
         return newValue;
     }


A subtle spec. question is whether the bold "*oldValue != null*" 
condition in compute() and merge() is to be kept or removed . So if the 
null return from the remappingFunction tries to remove the null value in 
possible old mapping or not (the looping variants do not remove it). 
That's a subtle divergence of looping atomic implementation from the 
spec., which currently says:

      * <p>If the function returns {@code null}, the mapping is removed (or
      * remains absent if initially absent).

So either we change the spec. to say that the possible mapping to null 
value remains if function returns null, or we must explicitly state in 
ConcurrentMap's overriden default method that it is not entirely by the 
spec.

A less subtle question is whether in general the spec. and the 
implementation for new default methods in Map should be kept in-sync 
with the implementation of their overriden counterparts in ConcurrentMap 
as far as null values in existent mappings are concerned (all other 
aspects of nulls be same). You suggested that for 
ConcurrentMap.getOrDefault() the implementation could diverge from the 
spec. to allow atomic implementation with a note in javadoc that any 
ConcurrentMap implementation that allows null values (which is currently 
non-existent) should override the method and implement it by the spec. 
In other words, the javadoc for default implementation of 
ConcurrentMap.getOrDefault() would explicitly state that it is *not* 
implemented by the spec. as far null values in existent mappings is 
concerned. If we accept this "divergence" of default implementation from 
the spec. (which does not affect existent ConcurrentMap implementations 
in any way) then such subtle differences are permissible, otherwise we 
should tailor the spec. so that it can be obeyed by both Map and 
ConcurrentMap default implementations.

The most important aspect I think is that all default methods in 
ConcurrentMap be atomic. The existent null values aspect is not more 
important than atomicity.

I also just noticed the following:

Map.putIfAbsent:

  627      * @return the previous value associated with the specified key, or
  628      **{@code 1}*  if there was no mapping for the key.
  629      *         (A <tt>null</tt> return can also indicate that the map
  630      *         previously associated <tt>null</tt> with the key,
  631      *         if the implementation supports null values.)


"1"?



Regards, Peter

>
> On Apr 8 2013, at 11:07 , Mike Duigou wrote:
>
>> Hello all;
>>
>> This is a combined review for the new default methods on the java.util.Map interface being added for the JSR-335 lambda libraries. The reviews are being combined because they share a common unit test.
>>
>> http://cr.openjdk.java.net/~mduigou/JDK-8010122/0/webrev/
>>
>> 8004518: Add in-place operations to Map
>> forEach()
>> replaceAll()
>>
>> 8010122: Add atomic operations to Map
>> getOrDefault()
>> putIfAbsent()          *
>> remove(K, V)
>> replace(K, V)
>> replace(K, V, V)
>> compute()              *
>> merge()                *
>> computeIfAbsent()      *
>> computeIfPresent()     *
>>
>> The * operations treat null values as being absent. (ie. the same as there being no mapping for the specified key).
>>
>> The default implementations provided in Map are overridden in HashMap for performance purposes, in Hashtable for atomicity and performance purposes and in Collections for atomicity.
>>
>> Mike