hg: lambda/lambda/jdk: bug fixes and unit test for Map Defaults

[Peter Levart]

Hi Mike,

I see default Map.computeIfAbsent has been chosen to not be atomic and 
rather support null values more naturally. That's ok if JDK 
ConcurrentMap implementations provide atomic overrides. Other 3rd party 
implementations will follow soon.

  But I have doubts about default Map.compute(). On one hand it contains 
the usual disclaimer:

       900      * <p>The default implementation makes no guarantees about
       901      * synchronization or atomicity properties of this method 
or the
       902      * application of the remapping function. Any class 
overriding
       903      * this method must specify its concurrency properties.  In
       904      * particular, all implementations of subinterface {@link
       905      * java.util.concurrent.ConcurrentMap} must document 
whether the
       906      * function is applied exactly once atomically. Any class 
that
       907      * permits null values must document whether and how this 
method
       908      * distinguishes absence from null mappings.

...but on the other hand it tries to be smart:

       924      * In concurrent contexts, the default implementation may 
retry
       925      * these steps when multiple threads attempt updates.

Now this last sentence may indicate that a ConcurrentMap implementation 
that does not override compute() might safely be used with default 
Map.compute() and be atomic. It was atomic until putIfAbsent() was 
replaced with plain put() to correct the "existing null value" behavior. 
The retry-loop is only needed when the optimistic operation is not 
successful. But put() is not optimistic. It always "succeeds". And 
retrying after the put() only makes things worse: "Oh, somebody was 
quicker than me and I have just overwritten his value - never mind, I'll 
try to make some more damage in next loop..."

If the damage was done already, then there's no point in repeating the 
loop. Further, what's the point in using optimistic operations: 
replace(key, oldValue, newValue) and remove(key, oldValue) with 
retry-loop on one hand and then just stomping over with put() on the 
other hand. If the default Map.compute() method is declared non-atomic, 
then plain put(key, newValue) instead of replace(key, oldValue, 
newValue) and remove(key) instead of remove(key, oldValue) could be used 
and no retry-loop...

The same goes for default Map.merge().

Regards, Peter

P.S. What do you think about changing the specification of putIfAbsent 
to always overwrite null values? It could make all these things simpler 
and more consistent. And it would not break anything.


On 03/25/2013 08:21 PM, mike.duigou at oracle.com wrote:
> Changeset: c8d40b7e6de3
> Author:    mduigou
> Date:      2013-03-20 20:32 -0700
> URL:       http://hg.openjdk.java.net/lambda/lambda/jdk/rev/c8d40b7e6de3
>
> bug fixes and unit test for Map Defaults
>
> ! src/share/classes/java/util/HashMap.java
> ! src/share/classes/java/util/Map.java
> + test/java/util/Map/Defaults.java
>
>