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

Mike Duigou mike.duigou at oracle.com
Tue Mar 26 23:00:59 PDT 2013


I've pushed another update that has putIfAbsent treat null values as absent. This allows putIfAbsent to be restored in a couple of the defaults. This feels like the right thing to do though some null-lover will eventually be unhappy that his null value got trampled. The remove() and replace() methods still distinguish between absent keys and present keys with null values. I think this is appropriate.

The alternative to treating null values as absent is to add additional code to distinguish between absent and nulls values everywhere. I attempted this in 

http://hg.openjdk.java.net/lambda/lambda/jdk/rev/8561d74a9e8f

It wasn't much more successful because the value mappers use null for signalling.

The current specification and behaviour seems about as good as can be achieved while still tolerating nulls and without introducing Optional.

Mike

On Mar 25 2013, at 16:41 , Mike Duigou wrote:

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/lambda-libs-spec-experts/attachments/20130326/06c52bec/attachment.html 


More information about the lambda-libs-spec-experts mailing list