Skip to content
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

Bbloom is unsafe #1005

Closed
Stebalien opened this issue Aug 23, 2019 · 4 comments · Fixed by hypermodeinc/ristretto#36
Closed

Bbloom is unsafe #1005

Stebalien opened this issue Aug 23, 2019 · 4 comments · Fixed by hypermodeinc/ristretto#36
Assignees
Labels
kind/bug Something is broken. priority/P1 Serious issue that requires eventual attention (can wait a bit)

Comments

@Stebalien
Copy link
Contributor

The IPFS project has a fork (https://github.com/ipfs/bbloom) that fixes these issues but we've made some significant API changes that make these fixes a bit difficult to upstream. These changes should either be upstreamed manually or you should consider switching to the fork.

Benchcmp:

BenchmarkM_New-4                        14633         16021         +9.49%
BenchmarkM_Clear-4                      2283          2004          -12.22%
BenchmarkM_Add-4                        2324843       1849145       -20.46%
BenchmarkM_Has-4                        2162063       1183050       -45.28%
BenchmarkM_AddIfNotHasFALSE-4           2464076       2224163       -9.74%
BenchmarkM_AddIfNotHasClearTRUE-4       2471798       2240655       -9.35%
BenchmarkM_AddTS-4                      5748868       3423050       -40.46%
BenchmarkM_HasTS-4                      5912475       1869951       -68.37%
BenchmarkM_AddIfNotHasTSFALSE-4         5841705       3982971       -31.82%
BenchmarkM_AddIfNotHasTSClearTRUE-4     6067134       4027179       -33.62%

(i.e., our fork is faster everywhere that counts but doesn't use unsafe)

@Stebalien Stebalien mentioned this issue Aug 23, 2019
13 tasks
@AndreasBriese
Copy link

...
using math/bits means using unsafe anyway. I prefer doing things transparent instead of obfuscating it.
I will look into the speed comparison.

@Stebalien
Copy link
Contributor Author

It uses the unsafe package internally but it uses it correctly. I only suggested switching libraries assuming that the changes from our bbloom fork aren't upstreamed. You appeared to be busy so I didn't want to block fixing these issues on upstreaming the necessary changes.

@Stebalien
Copy link
Contributor Author

@AndreasBriese has fixed upstream so updating to the latest master (of AndreasBriese/bbloom) should fix the issue.

@jarifibrahim
Copy link
Contributor

Badger uses dgraph-io/ristretto/z/bbloom - a fork of AndreasBriese/bbloom. https://github.com/dgraph-io/ristretto/tree/master/z should be updated accordingly.

@ashish-goswami ashish-goswami self-assigned this Aug 26, 2019
@ashish-goswami ashish-goswami added kind/bug Something is broken. priority/P1 Serious issue that requires eventual attention (can wait a bit) labels Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something is broken. priority/P1 Serious issue that requires eventual attention (can wait a bit)
Development

Successfully merging a pull request may close this issue.

4 participants