Hi Martin,
thanks for review and sponsorship. All fixed, details inlined.

new webrev: http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8210280/webrev.01/

On 12/12/18 3:55 AM, Martin Buchholz wrote:
Hi Michal, pleased to meet you.  I'll be your sponsor.

The test will need a legal header, presumably similar to others authored by
redhatters.
fixed


It is now possible to check in jmh microbenchmarks (but I've never done so
myself).
done


The current coding style is non-standard, but deliberate; avoid gratuitous
changes like
-                Node<K,V> e;
+                Node<K, V> e;
Fixed.
I insist on adding {} to if/else as it is extremely prone to errors. I can add it in different changeset if you like.


As author of concurrent/ConcurrentHashMap/WhiteBox.java ... > - I thought you'd need a @modules ... did this test actually pass?It pass but
with WARNING: An illegal reflective access operation has occurred. Fixed.

- you should have some kind of @summary that includes the word "whitebox"
fixed

- why not "throws ReflectiveOperationException" ?
fixed

- your reviewer is a bit on the autistic side, so please change /** to /*
on the @test comment (it's not javadoc!)
fixed (other tests in HashMap have also /**. I guess it does not worth the separate fix. or?)



On Tue, Dec 11, 2018 at 11:06 AM Michal Vala <mv...@redhat.com> wrote:

Hi,

I've been looking into this bug in HashMap where resize() is called
multiple
times when putting whole map into another.

I come up with following patch:
http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8210280/webrev.00/

I've tried to do it as little invasive as possible. New resize(int) method
is
called just from putMapEntries for now. Notice that method is called with
size
of the new map. I was experimenting with calling it with 'size()+s', but
this
leads to unwanted space allocation when inserted map rewrites a lot of
existing
keys.

I've benchmarked this with adding 10millions elements into existing map
and it
gets ~50% improvement:

unpatched
Benchmark                Mode  Cnt  Score   Error  Units
MyBenchmark.testMethod  thrpt   10  1.248 ± 0.058  ops/s

patched
Benchmark                Mode  Cnt  Score   Error  Units
MyBenchmark.testMethod  thrpt   10  1.872 ± 0.109  ops/s

public class MyBenchmark {
      static Map<Integer, Integer> mapTocopy = IntStream.range(1,
10_000_000).boxed().collect(Collectors.toMap(k -> k,
              k -> k));

      @Benchmark
      public int testMethod() {
          var map = new HashMap<Integer, Integer>();
          map.put(-5, -5);
          map.putAll(mapTocopy);
          return map.size();
      }
}

Any ideas for missed corner-cases or some good tests?

--
Michal Vala
OpenJDK QE
Red Hat Czech



--
Michal Vala
OpenJDK QE
Red Hat Czech

Reply via email to