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

[Joe Bowbeer]

I was not expecting Map to define a default impl. for what was originally a
ConcurrentMap method, btw.

If it does, however, and if Map supports null - as all the base Collections
do - then shouldn't all the default method implementations support null as
well? (Support is the correct term here, as the Collections spec makes no
mention of tolerate.)

In Map there is a distinction between !contains(key) and get(key)==null. I
think putIfAbsent is even defined in terms of contains() and not get().

How can we justify not fully supporting null in some Map methods?

I can't come up with a rationale or a loophole to exploit.
On Mar 27, 2013 12:01 AM, "Mike Duigou" <mike.duigou at oracle.com> wrote:

> 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/20130327/33644566/attachment.html