Use hash function to generate stable table types

[chuan]

See: https://gaiaplatform.atlassian.net/browse/GAIAPLAT-1389 for details on this workitem.

Some unit tests are updated due to the change of type generation algorithms. We can no longer assume type 1 or 2 always exists.

[senderista]

Given the fact that the chosen hash function actually affects correctness (not just performance as in a hash table), I am pretty uneasy about not directly controlling the hash function we use. It appears that the libstdc++ implementation of std::hash[0] uses MurmurHashUnaligned2 and libc++[1] uses MurmurHash2 or CityHash, all of which are state-of-the-art algorithms, but there are other issues which make using std::hash infeasible for this application:

1. There is no guarantee of hash stability between program executions. To quote the standard (https://en.cppreference.com/w/cpp/utility/hash):

Hash functions are only required to produce the same result for the same input within a single execution of a program; this allows salted hashes that prevent collision denial-of-service attacks.

This alone means that std::hash cannot be used in any application that requires stable mappings from inputs to outputs. (And in practice, you can expect unstable behavior, to thwart hash flooding attacks on hash tables.)

2. The other issue is that any STL implementation is free to change its std::hash implementation in any release, because by 1) it has no stability guarantees even between program executions using the same version of an STL implementation, so a fortiori it has no obligation to keep the hash function implementation the same between releases.

The answer is simple: use a hash function implementation that we directly control and whose stability we can therefore guarantee. There are many good free non-cryptographic hash functions now, but I don't think you can go wrong with either MurmurHash3 or xxHash. OTOH, for this application collision-resistance is critical, and there are likely no downsides to just using a cryptographic hash, so unless there are performance requirements that make it infeasible, I think we might as well just use truncated SipHash or SHA2 or SHA3. Better safe than sorry.

[0]libstdc++ std::hash implementation:
https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/libsupc%2B%2B/hash_bytes.cc

[1]libc++ std::hash implementation:
https://github.com/llvm/llvm-project/blob/bcaa01752f5e45b9d1647aef16d74c748192f8a5/libcxx/include/__string#L1155

[senderista]

Per my long comment, I think we need to explicitly specify the hash function, and if performance allows it should be crypto-quality.

[yiwen-wong]

Per my long comment, I think we need to explicitly specify the hash function, and if performance allows it should be crypto-quality.

Any reason we need a crypto-quality hash? Or a hash in general? I would say any deterministic mapping function over the fully qualified symbol name should work...

[senderista]

Per my long comment, I think we need to explicitly specify the hash function, and if performance allows it should be crypto-quality.

Any reason we need a crypto-quality hash? Or a hash in general? I would say any deterministic mapping function over the fully qualified symbol name should work...

Of course any 1:1 function from symbol names to integers would work, but the universe of symbol names under some reasonable length has cardinality >> 2^32, so if you don't want to materialize this mapping explicitly and manage it in a centralized fashion (e.g. via an autoincrement assignment of integers to symbol names, maintained in some master database), then I don't see an alternative to a hash function (and also constraints on the number of hashes you generate, as we've discussed). You don't strictly need a crypto-quality hash function for this, just one with sufficient (empirically demonstrated) collision resistance. But IMO we may as well use a (truncated) crypto hash function if our performance requirements allow it, for guaranteed rather than "good enough" collision resistance.

[chuan]

I have switched to our own murmur3 hash. I don't think any of the crypto hash properties are needed here, and I certainly don't want to take a dependency on any crypto library or maintain a crypto function myself for a temporary solution.

[simone-gaia]

LGTM as long as the smart ones are OK with it :)

[senderista]

I'm fine with copying a fixed reference implementation into our codebase, but I think it should be relocated to a common source location and library. I agree that MurmurHash3 should be sufficient for our requirements.

Just noted a couple of typos otherwise.

[senderista]

Please put this in a separate file under gaia_internal/common.

[chuan]

Moved.

[senderista]

Hmm, I'm concerned about unaligned reads in this variant, because 1) that's undefined behavior (see e.g. https://discourse.julialang.org/t/problem-with-issue-murmurhash3-has-undefined-behavior/9807), and 2) it could cause hardware exceptions on certain ARM CPUs (which has apparently happened with MurmurHash), since UB means that alignment fixup code won't be generated (I don't know details well). How about MurmurHashAligned2?

(Re: MurmurHash3, the comment at https://github.com/aappleby/smhasher/blob/master/src/MurmurHash3.cpp#L52 seems to indicate that getblock32()/getblock64() are intended to be extensibility points for aligning reads, but we'd have to implement that ourselves, although that would be trivial with memcpy. Also, those may not be sufficient: aappleby/smhasher#74.)

[senderista]

Sorry, I just noticed that you already fixed the unaligned reads and writes in MurmurHash3_x86_32, nice job!

[senderista]

Oh, NVM, looks like you already handled unaligned reads, nice!

[senderista]

Oh, and you handled the unaligned write here too, nice!

[LaurentiuCristofor]

Why not just copy that implementation in its own file so we can reuse it elsewhere instead of having it embedded in this cpp file? Copying the implementation instead of adapting it would also make it easier to review the implementation, because we could just ensure that the copy is correct instead of walking through the adaptations to make sure that it doesn't break the original implementation.

[chuan]

It is adapted. I need to make a few adjustments. For example, handling unaligned reads as Tobin noted below.

[LaurentiuCristofor]

Even if the adaptation is needed, why not move it into its own file, so it can be reused elsewhere? Do we expect to replace this implementation with something else in the future?

[chuan]

Moved to gaia_internal/common

[senderista]

LGTM