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 <mvala@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