JDK-8210280 - Unnecessary reallocation when invoking HashMap.putAll()

[Michal Vala]

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 at 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