identityHash or scaledIdentityHash ?

> Maybe Levente will help me clarifying this...
> I encountered several different implementations:
>
> MessageTally, Semaphore, FutureMaker, LocatedMethod, TextAnchor
> hash
>    ^self identityHash
> (or ^someInstVar identityHash, which is equivalent)
>
> Object
> hash
>   ^self scaledIdentityHash
>
> The first does not scale well for large Set/Dictionary, and the second
> costs a little overhed on small (Set/Dictionary).
> My questions are:
> - are these different implementations deliberate ? For example knowing
> there won't be any large Set... In which case a comment would help

> I also noted that MethodDictionary does not override
> scanForEmptySlotFor: which is defined in term either of hash or
> scaledIdentityHash, rather than IdentityHash
> This is currently OK, because the unique path I found was:
>
> MethodDictionary>>#at:put
> -> HashCollection>>atNewIndex:put:
> -> MethodDictionary>>#grow
> OK, grow overrides super, otherwise:
> -> HashCollection>>#grow
> -> HashCollection>>#growTo:
> -> Dictionary>>#noCheckNoGrowFillFrom:
> -> HashCollection>>#scanForEmptySlotFor:
> -> Symbol>>#hash
>
> But it's a potential tricky trap for future.
>

> On Thu, 24 Dec 2009, Nicolas Cellier wrote:
>
>> Maybe Levente will help me clarifying this...
>> I encountered several different implementations:
>>
>> MessageTally, Semaphore, FutureMaker, LocatedMethod, TextAnchor
>> hash
>>   ^self identityHash
>> (or ^someInstVar identityHash, which is equivalent)
>>
>> Object
>> hash
>>  ^self scaledIdentityHash
>>
>> The first does not scale well for large Set/Dictionary, and the second
>> costs a little overhed on small (Set/Dictionary).
>> My questions are:
>> - are these different implementations deliberate ? For example knowing
>> there won't be any large Set... In which case a comment would help
>
> No.
>
>> - or should we always use the scaledIdentityHash to hash ? (and let
>> identityHash for printing and a few well commented exceptional cases)
>
> Exactly.
>
> I quickly checked the classes and I think that:
> - all of the above should be #scaledIdentityHash
> - optionally MessageTally >> #hash should use the process's hash,
>  since #= also compares the process variable
>
>>
>> Beside, apart printing usage, I note these two usages of IdentityHash as
>> hash:
>> MethodDictionary scanFor: (I guess we don't expect more than 4096
>> messages too often).
>
> MethodDictionary's array's size is a power of two, and #scaledIdentityHash
> is just #identityHash bitShift: 18. Using #scaledIdentityHash would degrade
> the performance to the minimum. The use of #hash instead of #identityHash
> could help with large MethodDictionaries, since the keys are Symbols, but
> the current code assumes that the size will never reach 4096. And that's
> acceptable IMO.
>

>> Player recaptureUniqueCostumes
>>
>
> That should use #scaledIdentityHash, since the set defined there mimics an
> IdentitySet.
>
>> I also noted that MethodDictionary does not override
>> scanForEmptySlotFor: which is defined in term either of hash or
>> scaledIdentityHash, rather than IdentityHash
>> This is currently OK, because the unique path I found was:
>>
>> MethodDictionary>>#at:put
>> -> HashCollection>>atNewIndex:put:
>> -> MethodDictionary>>#grow
>> OK, grow overrides super, otherwise:
>> -> HashCollection>>#grow
>> -> HashCollection>>#growTo:
>> -> Dictionary>>#noCheckNoGrowFillFrom:
>> -> HashCollection>>#scanForEmptySlotFor:
>> -> Symbol>>#hash
>>
>> But it's a potential tricky trap for future.
>>
>
> Well, maybe. MethodDictionary has a different structure than other
> HashedCollections and because of this it has to reimplement #grow and
> #rehash. #scanForEmptySlotFor: is only used by those two methods in general
> (WeakKeyDictionary is the only exception).
>