RFR: 8006593: Performance and compatibility improvements to hash based Map implementations

[Peter Levart]

Yes, there are two thing here, as I noted. The 1st is compile-time 
constant replacement, the second is modifying the final field, which 
might be ok in the constructor of the object or at the deserialization 
time (as in readObject()), but you also had it assigned during 
re-hashing - at that time, I think, there could be code transformations 
that used cached value instead of re-reading the field...

It's better this way with normal field. I doubt there is any performance 
penalty...

Regards, Peter

On 03/05/2013 01:20 AM, Mike Duigou wrote:
> You are correct that there is a problem here but not because of the 
> unsafe.putIntVolatile. The problem is that hashSeed = 0; is a compile 
> time constant and field accesses are elided in the compiled methods.
>
> The idiom of setting a final field via unsafe.putIntVolatile is 
> adapted from the deserialization code of ConcurrentHashMap. I've 
> assumed that the memory model is correct. Objects being deserialized 
> usually aren't visible to other threads (and the implementation of 
> HashMap doesn't allow itself to be made visible to other threads 
> during deserialization). I am less certain about the 
> unsafe.putIntVolatile in initHashSeedAsNeeded and may just make 
> hashSeed non-final.
>
> I will correct the patch so that hashSeed isn't a compile time constant.
>
> :-(
>
> Mike
>
> On Mar 4 2013, at 14:14 , Peter Levart wrote:
>
>> Hi mike,
>>
>> I doubt (haven't tried it really with your code) that hashSeed will 
>> be seen by code to be anything else but 0, since it is initialized to 
>> a constant value. For example, this code:
>>
>> public class ModifyingFinalTest {
>>     static final Unsafe unsafe;
>>     static final long valueOffset;
>>
>>     static {
>>         try {
>>             Field f = Unsafe.class.getDeclaredField("theUnsafe");
>>             f.setAccessible(true);
>>             unsafe = (Unsafe)f.get(null);
>>             valueOffset = 
>> unsafe.objectFieldOffset(ModifyingFinalTest.class.getDeclaredField("value"));
>>         }
>>         catch (IllegalAccessException | NoSuchFieldException e) {
>>             throw new Error(e);
>>         }
>>     }
>>
>>     final int value = 0;
>>
>>     void test() {
>>         unsafe.putIntVolatile(this, valueOffset, 1);
>>         printValue();
>>         unsafe.putIntVolatile(this, valueOffset, 2);
>>         printValue();
>>         unsafe.putIntVolatile(this, valueOffset, 3);
>>         printValue();
>>     }
>>
>>     void printValue() {
>>         System.out.println(value);
>>     }
>>
>>     public static void main(String[] args) {
>>         new ModifyingFinalTest().test();
>>     }
>> }
>>
>>
>> Prints:
>>
>> 0
>> 0
>> 0
>>
>>
>> It's a different thing, if the initialization is changed to:
>>
>> final int value = "".length();
>>
>> But I don't know if each access in source is actually guaranteed to 
>> translate to a real read of field in this case either. Is 
>> Unsafe.putIntVolatile() making this happen somehow magically?
>>
>> Regards, Peter
>>
>>
>> On 03/04/2013 09:21 PM, Mike Duigou wrote:
>>> Hello all;
>>>
>>> The alternative hashing implementation introduced in 7u6 added an unfortunate bottleneck to the initialization of HashMap and Hashtable instances. This patch avoids the performance bottleneck of using a shared Random instance by using a ThreadLocalRandom instead.
>>>
>>> Along with this change are some additional performance improvements to further reduce the overhead of the alternative hashing feature and generally improve the performance of Hashtable or HashMap.
>>>
>>> http://cr.openjdk.java.net/~mduigou/JDK-8006593/3/webrev/
>>>
>>> Once review is completed here this patch will be proposed to JDK7u-dev for integration into the next 7u performance/feature release.
>>>
>>> Mike
>>>
>>
>