OpenHashMaps.mergePRIMITIVE: Avoid double-hashing key #337
Conversation
|
Looks good to me. Disambiguation will happen at the interface level, and it should be inlined. |
|
BTW, did you check that your code is actually executed (either by prints or by coverage tools)? The dispatch of that stuff is absolutely devilish... |
|
Thank you! You're right, I should check this is actually executing. I think I'll write a test with an object that counts how many times it was hashed, asserting it was just hashed once after. I'll just need to be a little careful about rehashing. |
|
You can also just use a coverage tool. Or add a print in the code and make a one-off test, checking that something is printed. |
|
I've confirmed it's executing. It's actually down from 3 hashes (get, containsKey, put) down to 1. I'm pretty happy to write a test for this; fastutil is a long-term project and it seems worth adding some test to stop this unintentionally regressing in future. |
|
I think I can add a test in |
3 is too many times. In a future commit, we will reduce this. The goal here is to demonstrate that the future commit works.
Previously, we were hashing the key 3x: - get - containsKey - put Now, we override mergeDouble with a hashamp-specific implementation that hashes the key once. Towards vigna#336
|
PTAL; I've added a test in commit 1, then reduced the allocations in commit 2. |
|
Thank you for the review! |
The
#if VALUES_PRIMITIVE && ! VALUE_CLASS_Booleancomes from the superclass in Map.drv:fastutil/drv/Map.drv
Line 649 in 355d8eb
The implementation mostly is copied from the method below (
merge), with dead code removed becauseVALUES_PRIMITIVEis always true.I'm using the
METHOD_ARG_VALUE_BINARY_OPERATORso that we prefer using the JDK ${Primitive}BinaryOperator classes, and fall back to the fastutil primitive-BinaryOperators where not present in the JDK.I ran the junit tests locally and they pass.
Towards #336