Avoid getting hashCode of keys for empty ConcurrentHashMap

[Martin Buchholz]

Thanks!

I filed https://bugs.openjdk.java.net/browse/JDK-8244960
Looks like a good change.
(But we are more time-limited than usual these days)

On Wed, May 13, 2020 at 10:35 AM Christoph Dreis
<christoph.dreis at freenet.de> wrote:
>
> Hi,
>
> forgive me if this is not the correct list. I get confused where things about java.util.concurrent should be posted.
>
> I found an optimization opportunity in ConcurrentHashMap that seems relatively straight-forward.
>
> When ConcurrentHashMap.get() is called on an empty map, the key's hashCode() method is still called while it doesn't seem necessary to do so.
>
> With the attached patch I see the following results:
>
> Patched
> Benchmark                             Mode  Cnt   Score    Error   Units
> Benchmark.test                      avgt   10   2,713 ±  0,290   ns/op
> Benchmark.test:·gc.alloc.rate       avgt   10  ≈ 10⁻⁴           MB/sec
> Benchmark.test:·gc.alloc.rate.norm  avgt   10  ≈ 10⁻⁶             B/op
> MyBenchmark.test:·gc.count            avgt   10     ≈ 0           counts
>
> Before patch
> Benchmark                             Mode  Cnt   Score    Error   Units
> Benchmark.test                      avgt   10   3,759 ±  0,442   ns/op
> Benchmark.test:·gc.alloc.rate       avgt   10  ≈ 10⁻⁴           MB/sec
> Benchmark.test:·gc.alloc.rate.norm  avgt   10  ≈ 10⁻⁶             B/op
> Benchmark.test:·gc.count            avgt   10     ≈ 0           counts
>
> For completeness reasons, here is the benchmark I used:
>
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> public class MyBenchmark {
>
>     @State(Scope.Benchmark)
>     public static class ThreadState {
>
>         private Map<TestKey, String> map = new ConcurrentHashMap<>();
>         private TestKey key = new TestKey(Collections.singleton("test"));
>
>     }
>
>     @Benchmark
>     public String test(ThreadState threadState) {
>         return threadState.map.get(threadState.key);
>     }
>
> }
>
> Where TestKey is the following:
>
> public class TestKey {
>
>         private final Set<String> params;
>
>         public TestKey(Set<String> params) {
>                 this.params = params;
>         }
>
>         @Override
>         public int hashCode() {
>                 return this.params.hashCode();
>         }
>
> }
>
> This usually happens if the map is not populated yet - which is also the case for some JDK internal usages. But I've also seen cases where the map stays empty during runtime[1].
>
> If you think, this is worthwhile I'd appreciate if this patch could be sponsored.
>
> Cheers,
> Christoph
>
> [1] https://github.com/spring-projects/spring-framework/blob/master/spring-core/src/main/java/org/springframework/core/SimpleAliasRegistry.java#L49
>
>
> ========= PATCH ==========
>
> --- a/src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java   Wed May 13 16:18:16 2020 +0200
> +++ b/src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java   Wed May 13 19:15:46 2020 +0200
> @@ -932,10 +932,9 @@
>       * @throws NullPointerException if the specified key is null
>       */
>      public V get(Object key) {
> -        Node<K,V>[] tab; Node<K,V> e, p; int n, eh; K ek;
> -        int h = spread(key.hashCode());
> +        Node<K,V>[] tab; Node<K,V> e, p; int h, n, eh; K ek;
>          if ((tab = table) != null && (n = tab.length) > 0 &&
> -            (e = tabAt(tab, (n - 1) & h)) != null) {
> +            (e = tabAt(tab, (n - 1) & (h = spread(key.hashCode())))) != null) {
>              if ((eh = e.hash) == h) {
>                  if ((ek = e.key) == key || (ek != null && key.equals(ek)))
>                      return e.val;
>
>