[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D122974#3481269 <https://reviews.llvm.org/D122974#3481269>, @llunak wrote:

> In D122974#3480686 <https://reviews.llvm.org/D122974#3480686>, @dblaikie wrote:
>
>> In D122974#3424654 <https://reviews.llvm.org/D122974#3424654>, @JDevlieghere wrote:
>>
>>> 
>
> ...
>
>>>   struct HashedStringRef {
>>>     const llvm::StringRef str;
>>>     const unsigned full_hash;
>>>     const uint8_t hash; 
>>>     HashedStringRef(llvm::StringRef s) : str(s), full_hash(djbHash(str)), hash(hash(str)) {}
>>>   }
>
> ...
>
>> The external code shouldn't be able to create their own (ctor private/protected, etc) - the only way to get one is to call `StringMap` to produce one.
>
> But what if I don't want StringMap to compute the hash at all? There's still that 10-15% of CPU time spent in djbHash() when debug info uses exactly[*] that (and LLDB's index cache could be modified to store it). Which means it can be useful for StringMap to allow accepting precomputed hash, and then what purpose will that HashedStringRef have?

If that happens, yeah, there's probably no point protecting this API - though I'd be more cautious of that change/any change that supports serializing the hash as this could end up causing StringMap to have to have stability guarantees that might be unfavorable for other uses (see ABSL maps that intentionally randomize/reseed each instance to ensure users aren't depending on undocumented guarantees - this gives them the flexibility to update the hash algorithm with something better in the future without breaking users who might be serializing the hashes/relying on iteration order/etc)

If we want a structure that can use a stable hash - it might be at that point that we move the hash support entirely out of StringMap and make it pluggable, with the default implementation doing djbHash as before, and even the new "stable" one doing that too, but doing it explicitly/documenting what guarantees it requires (stability within a major version? Across major versions?)

& then I guess what the API looks like is still an open question - perhaps the default trait (the one without any stability guarantees) could have a private implementation and expose that to StringMap via friendship. The stable implementation can have a public implementation for hashing & then an API like the one proposed where they can be passed into StringMap (yeah, without any of the handle safety previously proposed) - and assert when it's passed in that it matches what the trait provides (under EXPENSIVE_CHECKS, probably).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122974/new/

https://reviews.llvm.org/D122974