RFR: 8004518 & 8010122 : Default methods on Map

[Mike Duigou]

On Apr 8 2013, at 19:22 , David Holmes wrote:

> Hi Mike,
> 
> Looking only at Map itself for now.
> 
> On 9/04/2013 4:07 AM, Mike Duigou wrote:
>> Hello all;
>> 
>> This is a combined review for the new default methods on the java.util.Map interface being added for the JSR-335 lambda libraries. The reviews are being combined because they share a common unit test.
>> 
>> http://cr.openjdk.java.net/~mduigou/JDK-8010122/0/webrev/
> 
> General style issues:
> 
> - spaces after keyword ie "if (x == null)" not "if(x == null)"

Fixed. I am sorry this keeps coming up. I am loathe to run an automatic formatter on any JDK code.

> - comparisons against constants/null put the constant/null on the right ie "if (x == null)" not "if (null == x)"

Do I shall.

> 
> (where has our style guide gone? I can't find it on internal or external wikis :( )

This one? http://www.oracle.com/technetwork/java/codeconv-138413.html

I am unaware of any other guide that's official policy.

> 
>> 8004518: Add in-place operations to Map
>>  forEach()
>>  replaceAll()
> 
> Both of those contain the boilerplate text:
> 
>  * <p>The default implementation makes no guarantees about
>  * synchronization or atomicity properties of this method. Any
>  * class overriding this method must specify its concurrency
>  * properties. In particular, all implementations of
>  * subinterface {@link java.util.concurrent.ConcurrentMap}
>  * must ensure that this operation is performed atomically.
> 
> but these methods are not, and can not be, atomic in ConcurrentMap

I've altered the text to incorporate your suggestions. It now read:

     * <p>The default implementation makes no guarantees about synchronization
     * or atomicity properties of this method. Any class which wishes to provide
     * specific synchronization, atomicity or concurrency behaviour should
     * override this method.

I would like to use this same text on the other methods as well.

> 
> forEach and replaceAll are very similar in terms of taking and applying a "operation" to each entry, yet their descriptions use a completely different style.

Written by different people independently.

> forEach describes thing in general terms while replaceAll talks about calling the functions' apply method with the current entry's key and value. I would suggest for replaceAll:
> 
> "Replaces each entry's value with the result of invoking the given function on that entry, in the order entries are returned by an entry set iterator, until all entries have been processed or the function throws an exception."
> 
> This is also makes "replace" the subject rather than "apply".

Yes, reads better to me.

> 
> forEach doesn't declare the IllegalStateException that getKey and getValue can throw.

Since forEach isn't supposed to mutate the map this shouldn't happen. It could only happen through concurrent modification. I've added a catch of the IllegalStateException and throw CME with the ISE as the cause to both forEach and replaceAll.

> Some/many of the @throws text has obviously been copied from the Map.Entry methods eg:
> 
> * @throws ClassCastException if the class of the specified value
> *         prevents it from being stored in the backing map
> 
> but when put into Map itself they should not be referring to "the backing map" as there is no "backing map". Further we have inconsistent terminology being used, eg getOrDefault has:
> 
> * @throws ClassCastException if the key is of an inappropriate type for
> * this map
> 
> and then there is a third variant in other methods:
> 
>  * @throws ClassCastException if the class of the specified key or value
>  *         prevents it from being stored in this map
>  *         (<a href="Collection.html#optional-restrictions">optional</a>)
> 
> These should all have the same basic wording, differing only in key/value.

agreed. The third variant is now used consistently.


> 
>> 8010122: Add atomic operations to Map
> 
> Meaning "backport some operations from ConcurrentMap" - they aren't actually atomic in Map.

OK. 
> 
>>  getOrDefault()
> 
> No comment
> 
>>  putIfAbsent()          *
> 
> The default implementation throws ConcurrentModificationException but this is not declared in the spec.

fixed

> 
>>  remove(K, V)
>>  replace(K, V)
>>  replace(K, V, V)
> 
> No comments
> 
>>  compute()              *
>>  merge()                *
>>  computeIfAbsent()      *
>>  computeIfPresent()     *
> 
> The following generally apply to this group of methods.
> 
> As Peter already stated the spec:
> 
> * <p>If the function returns {@code null}, the mapping is removed (or
> * remains absent if initially absent).
> 
> is somewhat unclear. The parenthesized part is not connected to the function returning null or otherwise; as the function won't be called.

Corrected.

> I find the spec for these rather confusing from a concurrency perspective - this non-concurrent interface seems to be trying to say too much about how a concurrent interface should specify behaviour. Why does it need to say:
> 
>  * In concurrent contexts, the default implementation may retry
>  * these steps when multiple threads attempt updates.

For default on Map I am starting to think that throwing that CME rather than looping is the right choice. The retry behaviour seems to be counter the basic behaviour of non-concurrent implementations. The retry behaviour will just hide usage that's otherwise unsafe when used with non-concurrent implementations.

The concurrent implementations don't use these defaults so there's no problem switching to throwing CME.

Opinions?

> ? Note computeIfAbsent does not say the same thing.

It doesn't attempt to loop.

> 
> The @implSpec does not match the actual implementation. It looks to me like these implementations are trying to cater for concurrent situations - hence the loop. That's okay but then the implSpec should identify that fact.

Corrected several of the impl descriptions.

> 980  * be of use when combining multiple mapped values for a key.  For
> 981  * example. to either create or append a {@code String msg} to a
> 
> Period after example should be a comma.

fixed.

> 
> Cheers,
> David
> 
>> The * operations treat null values as being absent. (ie. the same as there being no mapping for the specified key).
>> 
>> The default implementations provided in Map are overridden in HashMap for performance purposes, in Hashtable for atomicity and performance purposes and in Collections for atomicity.
>> 
>> Mike
>> 
>>