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

[Sam Pullara]

Is there some reason it doesn't call replace() when the old value is null? Calling put seems wrong to me.

Sam

On Mar 25, 2013, at 2:57 PM, Peter Levart <peter.levart at gmail.com> wrote:

> 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
>> 
>> 
> 
>