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

[Mike Duigou]

My intention with the first cut of the unit test was to avoid changing the method specification while having consistent behaviour across all of the Map implementations. It's certainly possible I made mistakes and changed the contract of the defaults in ways I didn't intend. 

I think perhaps that changing putIfAbsent to allow replacement of null values would help a lot to making the operations more consistent. I am also going to look at Sam's suggestion of using replace() rather than put() in a few places. Using replace() almost ensures though that we'll introduce more retry loop implementations. This may be fine though.

Mike


On Mar 25 2013, at 14:57 , Peter Levart 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
>> 
>> 
>