New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Is it a bug of not checking against max bucket and lock power ? #125
Comments
@wenhaocs good catch. Do you mind sending a PR to address that and I can merge it. |
@sathyaphoenix Please let me know if you want to throw error explicitly and tell users the limit for #cacheEntries, or just implicitly set the bucketPower to be maximum like in the PR. |
@wenhaocs Shall we use std::min instead of std::max to pick the smaller value? |
To be consistent with the constructor, it makes sense to throw an error when the value is going to be incorrect rather than silently fix it. Most likely in this case the user gave a wrong input and it is better to abort rather than silently run with a misconfigured hash table. Also, like @jiayuebao pointed out, we should be doing a std::min instead of max if we were to adjust to a meaningful value. @wenhaocs can you add a simple unit test for this ? we can introduce a new .cpp here to add CacheAllocatorConfig unit tests. @jiayuebao please let us know if there is a better place to add the test. |
For unit test, we already have one here. And we can add more cases there. |
There is a way of setting AccessConfig via number of estimated cache entries. However, unlike directly passing bucketPower and lockPower, the calculated bucketPower and lockPower is not checked against maximum. Is it a potential bug?
https://sourcegraph.com/github.com/facebook/CacheLib/-/blob/cachelib/allocator/CacheAllocatorConfig.h?L625
The text was updated successfully, but these errors were encountered: