RFR: 8004518 & 8010122 : Default methods on Map (original) (raw)

Peter Levart peter.levart at gmail.com
Thu Apr 11 08:46:29 UTC 2013

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 null return can also indicate that the map 630 * previously associated null 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

More information about the core-libs-dev mailing list