Hi Jacob,
Thanks for looking at this. What you have done looks complete (although it is
not necessary on HashTable since it tunnels directly to computeIfAbsent), and
the performance evaluation is appreciated.
While i have some sympathy with this, having written many such computeIfAbsent
calls, i don’t think the use of a method reference sufficiently warrants the
addition of such a helper method. We could of easily added such an overloaded
method when we added the original computeIfAbsent method but did not for
similar reasons (IIRC).
Further, i think it could be harmful to overload compute and computeIfPresent,
where such usage is likely less common may be incorrect leading to
unintentional bugs.
I hope that in the future we can use _ for lambda arguments we don’t care about
and/or shadowing will allow us to avoid the annoyance of using unique argument
names for nested expressions (i dunno if this will happen but pattern matching
may have some influence).
—
Often it's beneficial to discuss an idea before completion potentially saving
effort.
As a compensation, if you still want to contribute, how about we point you to
some starter bugs? (Note that bugs that involve new APIs, even small ones, will
involve more process to carefully review the API, whereas internal changes have
less process).
Hth,
Paul.
> On May 3, 2018, at 10:43 AM, Jacob Glickman <jhg...@bucknell.edu> wrote:
>
> Hello! This is my first patch, so I apologize in advance if I've done
> anything incorrectly (including botching the formatting of the mercurial
> diff below).
>
> I think it would be beneficial to have an overloaded `Map.computeIfAbsent`
> method where the computed value does not depend on the key; for that
> reason, I submitted [1]. By taking a `Supplier<? extends Value>`, this
> will allow users to utilize method references where applicable. For
> example, if they create a `Map<String, Collection<Integer>>`, then the
> following can be used when computing values:
>
> map.computeIfAbsent("key", ArrayList::new).add(1);
>
> I originally planned to add overloaded methods for `Map.computeIfPresent`
> and `Map.compute` as well, but it would make more sense for them to take a
> `Function` (instead of a `Supplier`) if the computed value does not depend
> on the key. If this patch is successful, then I'll go forward with that
> planned change. To clarify, an overloaded `Map.computeIfAbsent` method was
> the only change, along with the unit test.
>
> I used JMH to benchmark my implementation versus an inlined implementation
> (not calling the existing `Map.computeIfAbsent` with the result of the
> supplier), and the difference was negligible (maybe the compiler inlined
> it?). Please let me know if I should inline it so I can change my
> implementation.
>
> [1]: https://bugs.openjdk.java.net/browse/JDK-8202521
>
> ------------------------------------------------------
> # HG changeset patch
> # User jhg023
> # Date 1525366367 25200
> # Thu May 03 09:52:47 2018 -0700
> # Node ID 395def2871e8d077b382d722efc59b38373327d1
> # Parent ea246151be08995543d0c9281099608bc9207a19
> 8202521: Add overloaded methods of Map#compute, Map#computeIfAbsent,
> Map#computeIfPresent
> Summary: Added "Map.computeIfAbsent(K key, Supplier<? extends V> supplier)"
> Reviewed-by: N/A
> Contributed-by: Jacob Glickman <jhg...@bucknell.edu>
>
> diff -r ea246151be08 -r 395def2871e8
> src/java.base/share/classes/java/util/Hashtable.java
> --- a/src/java.base/share/classes/java/util/Hashtable.java Wed May 02
> 11:21:27 2018 -0700
> +++ b/src/java.base/share/classes/java/util/Hashtable.java Thu May 03
> 09:52:47 2018 -0700
> @@ -29,6 +29,7 @@
> import java.util.function.BiConsumer;
> import java.util.function.Function;
> import java.util.function.BiFunction;
> +import java.util.function.Supplier;
> import jdk.internal.misc.SharedSecrets;
>
> /**
> @@ -1054,6 +1055,21 @@
> * {@inheritDoc}
> *
> * <p>This method will, on a best-effort basis, throw a
> + * {@link java.util.ConcurrentModificationException} if the
> + * supplier modified this map during computation.
> + *
> + * @throws ConcurrentModificationException if it is detected that the
> + * supplier modified this map
> + */
> + @Override
> + public synchronized V computeIfAbsent(K key, Supplier<? extends V>
> supplier) {
> + return computeIfAbsent(key, k -> supplier.get());
> + }
> +
> + /**
> + * {@inheritDoc}
> + *
> + * <p>This method will, on a best-effort basis, throw a
> * {@link java.util.ConcurrentModificationException} if the remapping
> * function modified this map during computation.
> *
> diff -r ea246151be08 -r 395def2871e8
> src/java.base/share/classes/java/util/Map.java
> --- a/src/java.base/share/classes/java/util/Map.java Wed May 02 11:21:27
> 2018 -0700
> +++ b/src/java.base/share/classes/java/util/Map.java Thu May 03 09:52:47
> 2018 -0700
> @@ -28,6 +28,7 @@
> import java.util.function.BiConsumer;
> import java.util.function.BiFunction;
> import java.util.function.Function;
> +import java.util.function.Supplier;
> import java.io.Serializable;
>
> /**
> @@ -1010,6 +1011,85 @@
> }
>
> /**
> + * If the specified key is not already associated with a value (or is
> mapped
> + * to {@code null}), attempts to compute its value using the given
> supplier
> + * and enters it into this map unless {@code null}.
> + *
> + * <p>If the supplier returns {@code null}, no mapping is recorded.
> + * If the supplier itself throws an (unchecked) exception, the
> + * exception is rethrown, and no mapping is recorded. The most
> + * common usage is to construct a new object serving as an initial
> + * mapped value or memoized result that does not depend on the key,
> + * as in:
> + *
> + * <pre> {@code
> + * map.computeIfAbsent(key, () -> new Value());
> + * }</pre>
> + *
> + * <p>Or to implement a multi-value map, {@code Map<K,Collection<V>>},
> + * supporting multiple values per key:
> + *
> + * <pre> {@code
> + * map.computeIfAbsent(key, HashSet::new).add(v);
> + * }</pre>
> + *
> + * <p>The supplier should not modify this map during computation.
> + *
> + * @implSpec
> + * The default implementation is equivalent to the following steps for
> this
> + * {@code map}, then returning the current value or {@code null} if now
> + * absent:
> + *
> + * <pre> {@code
> + * if (map.get(key) == null) {
> + * V newValue = supplier.get();
> + * if (newValue != null)
> + * map.put(key, newValue);
> + * }
> + * }</pre>
> + *
> + * <p>The default implementation makes no guarantees about detecting
> if the
> + * supplier modifies this map during computation and, if appropriate
> + * reporting an error. Non-concurrent implementations should
> + * override this method and, on a best-effort basis, throw a
> + * {@code ConcurrentModificationException} if it is detected that the
> + * supplier modifies this map during computation. Concurrent
> + * implementations should override this method and, on a best-effort
> basis,
> + * throw an {@code IllegalStateException} if it is detected that the
> + * supplier modifies this map during computation and as a result
> + * computation would never complete.
> + *
> + * <p>The default implementation makes no guarantees about
> synchronization
> + * or atomicity properties of this method. Any implementation providing
> + * atomicity guarantees must override this method and document its
> + * concurrency properties. In particular, all implementations of
> + * subinterface {@link java.util.concurrent.ConcurrentMap} must
> document
> + * whether the supplier is applied once atomically only if the value
> + * is not present.
> + *
> + * @param key key with which the specified value is to be associated
> + * @param supplier the supplier to compute a value
> + * @return the current (existing or computed) value associated with
> + * the specified key, or null if the computed value is null
> + * @throws NullPointerException if the specified key is null and
> + * this map does not support null keys, or the supplier
> + * is null
> + * @throws UnsupportedOperationException if the {@code put} operation
> + * is not supported by this map
> + * (<a
> href="{@docRoot}/java.base/java/util/Collection.html#optional-restrictions">optional</a>)
> + * @throws ClassCastException if the class of the specified key or
> value
> + * prevents it from being stored in this map
> + * (<a
> href="{@docRoot}/java.base/java/util/Collection.html#optional-restrictions">optional</a>)
> + * @throws IllegalArgumentException if some property of the specified
> key
> + * or value prevents it from being stored in this map
> + * (<a
> href="{@docRoot}/java.base/java/util/Collection.html#optional-restrictions">optional</a>)
> + * @since 11
> + */
> + default V computeIfAbsent(K key, Supplier<? extends V> supplier) {
> + return computeIfAbsent(key, k -> supplier.get());
> + }
> +
> + /**
> * If the value for the specified key is present and non-null,
> attempts to
> * compute a new mapping given the key and its current mapped value.
> *
> diff -r ea246151be08 -r 395def2871e8 test/jdk/java/util/Map/Defaults.java
> --- a/test/jdk/java/util/Map/Defaults.java Wed May 02 11:21:27 2018
> -0700
> +++ b/test/jdk/java/util/Map/Defaults.java Thu May 03 09:52:47 2018
> -0700
> @@ -25,7 +25,7 @@
> * @test
> * @bug 8010122 8004518 8024331 8024688
> * @summary Test Map default methods
> - * @author Mike Duigou
> + * @author Mike Duigou, Jacob Glickman
> * @run testng Defaults
> */
>
> @@ -301,6 +301,27 @@
> assertSame(map.get(null), EXTRA_VALUE, "not expected value");
> }
>
> + @Test(dataProvider = "Map<IntegerEnum,String> rw=true keys=withNull
> values=withNull")
> + public void testComputeIfAbsentNullsSupplier(String description,
> Map<IntegerEnum, String> map) {
> + // null -> null
> + assertTrue(map.containsKey(null), "null key absent");
> + assertNull(map.get(null), "value not null");
> + assertSame(map.computeIfAbsent(null, () -> null), null, "not
> expected result");
> + assertTrue(map.containsKey(null), "null key absent");
> + assertNull(map.get(null), "value not null");
> + assertSame(map.computeIfAbsent(null, () -> EXTRA_VALUE),
> EXTRA_VALUE, "not mapped to result");
> + // null -> EXTRA_VALUE
> + assertTrue(map.containsKey(null), "null key absent");
> + assertSame(map.get(null), EXTRA_VALUE, "not expected value");
> + assertSame(map.remove(null), EXTRA_VALUE, "removed unexpected
> value");
> + // null -> <absent>
> + assertFalse(map.containsKey(null), "null key present");
> + assertSame(map.computeIfAbsent(null, () -> EXTRA_VALUE),
> EXTRA_VALUE, "not mapped to result");
> + // null -> EXTRA_VALUE
> + assertTrue(map.containsKey(null), "null key absent");
> + assertSame(map.get(null), EXTRA_VALUE, "not expected value");
> + }
> +
> @Test(dataProvider = "Map<IntegerEnum,String> rw=true keys=all
> values=all")
> public void testComputeIfAbsent(String description, Map<IntegerEnum,
> String> map) {
> // 1 -> 1
> @@ -321,6 +342,25 @@
> }
>
> @Test(dataProvider = "Map<IntegerEnum,String> rw=true keys=all
> values=all")
> + public void testComputeIfAbsentSupplier(String description,
> Map<IntegerEnum, String> map) {
> + // 1 -> 1
> + assertTrue(map.containsKey(KEYS[1]));
> + Object expected = map.get(KEYS[1]);
> + assertTrue(null == expected || expected == VALUES[1], description
> + String.valueOf(expected));
> + expected = (null == expected) ? EXTRA_VALUE : expected;
> + assertSame(map.computeIfAbsent(KEYS[1], () -> EXTRA_VALUE),
> expected, description);
> + assertSame(map.get(KEYS[1]), expected, description);
> +
> + // EXTRA_KEY -> <absent>
> + assertFalse(map.containsKey(EXTRA_KEY));
> + assertNull(map.computeIfAbsent(EXTRA_KEY, () -> null));
> + assertFalse(map.containsKey(EXTRA_KEY));
> + assertSame(map.computeIfAbsent(EXTRA_KEY, () -> EXTRA_VALUE),
> EXTRA_VALUE);
> + // EXTRA_KEY -> EXTRA_VALUE
> + assertSame(map.get(EXTRA_KEY), EXTRA_VALUE);
> + }
> +
> + @Test(dataProvider = "Map<IntegerEnum,String> rw=true keys=all
> values=all")
> public void testComputeIfAbsentNullFunction(String description,
> Map<IntegerEnum, String> map) {
> assertThrowsNPE(() -> map.computeIfAbsent(KEYS[1], null));
> }
> @@ -370,7 +410,7 @@
> assertThrowsNPE(() -> map.computeIfPresent(KEYS[1], null));
> }
>
> - @Test(dataProvider = "Map<IntegerEnum,String> rw=true keys=withNull
> values=withNull")
> + @Test(dataProvider = "Map<IntegerEnum,String> rw=true keys=withNull
> values=withNull")
> public void testComputeNulls(String description, Map<IntegerEnum,
> String> map) {
> assertTrue(map.containsKey(null), "null key absent");
> assertNull(map.get(null), "value not null");
