Add single value cache to BloomFilter hash calculation #73103
Conversation
Although this isn't horribly expensive, I noticed that we typically call BloomFilter.ProbablyContains many times with the same input string. This string is then hashed a number of times (about 13 times if the string is present, less if not). Instead, we can cache what these hashes are as almost all created bloom filters will calculate the same hashes. Testing yielded around a 99% hit rate. Should have a *slight* positive effect for find references and other scenarios using the bloom filters.
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
|
|
||
| if (cachedHash == null | ||
| || cachedHash._isCaseSensitive != filter._isCaseSensitive | ||
| || cachedHash._hashes.Length < filter._hashFunctionCount |
There was a problem hiding this comment.
Why is this a < and not a !=?
There was a problem hiding this comment.
Because a longer hash of arrays is fine. The caller may not use all the values in the array we hand back, but that's not a big deal.
| /// <summary> | ||
| /// Provides mechanism to efficiently obtain bloom filter hash for a value. Backed by a single element cache. | ||
| /// </summary> | ||
| internal class BloomFilterHash |
|
|
||
| for (var i = 0; i < filter._hashFunctionCount; i++) | ||
| hashBuilder.Add(filter.GetBitArrayIndex(value, i)); | ||
|
|
There was a problem hiding this comment.
This seems wrong (though it is early). The result of GetBitArrayIndex depends on internal state of the filter it doesn't seem like it could be used across filters.
| } | ||
|
|
||
| [Fact] | ||
| public void TestCacheAfterCalls() |
There was a problem hiding this comment.
We need tests with vastly different Bloom filters, demonstrating we get expected probability results.
There was a problem hiding this comment.
Is this a general request, or is this something you think needs testing in light of the code now using a cache?
|
I think there's a subtle, but very problematic bug. If I'm remembering how this works properly. |
…mod length to get the BitArrayIndex
CyrusNajmabadi
force-pushed
the
BloomFilterHashCache
branch
from
323b692
to
f47186e
Compare
CyrusNajmabadi
force-pushed
the
BloomFilterHashCache
branch
from
f47186e
to
ede3bb2
Compare
| public bool ProbablyContains(string value) | ||
| { | ||
| var hashes = BloomFilterHash.GetOrCreateHashArray(value, _isCaseSensitive, _hashFunctionCount); |
There was a problem hiding this comment.
ok. this is subtle, and needs docs. mention explicitly that hashes may contain more hash values than the _hashFunctionCount passed in. But that's ok as the first _hashFunctionCount hashes are guaranteed to be the same due to how ComputeHash works.
| { | ||
| var cachedHash = s_cachedHash; | ||
|
|
||
| // Not an equivalency check on the hashFunctionCount as a longer array is ok. |
There was a problem hiding this comment.
i'd prefer a longer explanation of why it isok. specifically that when getting the hashes that you always get hte same prefix of hashes regardless of how many hash function counts you ask for. In other words, the hash[0] is always hte same across all the arrays, as long as you have the same value and case sensitivity, and same for hash[1], and s on.
| /// we put those values into a simple cache and see if it can be used before calculating. | ||
| /// Local testing has put the hit rate of this at around 99%. | ||
| /// </summary> | ||
| public static ImmutableArray<int> GetOrCreateHashArray(string value, bool isCaseSensitive, int hashFunctionCount) |
There was a problem hiding this comment.
this should mention you can get a larger array, but should only use up to the first hashFunctionCount entries in it.
CyrusNajmabadi
force-pushed
the
BloomFilterHashCache
branch
from
1975bb8
to
7117760
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Although this isn't horribly expensive, I noticed that we typically call BloomFilter.ProbablyContains many times with the same input string. This string is then hashed a number of times (about 13 times if the string is present, less if not). Instead, we can cache what these hashes are as almost all created bloom filters will calculate the same hashes.
Testing yielded around a 99% hit rate. Should have a slight positive effect for find references and other scenarios using the bloom filters.