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

Conservative bloomfilter #42

Merged
merged 19 commits into from
Nov 13, 2022
Merged

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Aug 22, 2022

This takes #3 and fumpts it, and adds a few other known-safe tweaks.

@faddat faddat marked this pull request as draft August 22, 2022 11:12
@faddat faddat marked this pull request as ready for review August 22, 2022 11:35
@faddat
Copy link
Contributor Author

faddat commented Aug 22, 2022

Note to self:

cosmos-db/goleveldb.go

Lines 27 to 37 in e3cc080

func NewGoLevelDB(name string, dir string) (*GoLevelDB, error) {
options := &opt.Options{
BlockSize: 64, // this is for compatibility with raid 0 systems
Filter: filter.NewBloomFilter(10),
OpenFilesCacheCapacity: 8192,
BlockCacheCapacity: opt.GiB,
WriteBuffer: 64 * opt.MiB,
DisableSeeksCompaction: true,
}
return NewGoLevelDBWithOpts(name, dir, options)
}

The block size setting may relieve pressure on raid zero, make lvm unnecessary and improve cloud perf.

@faddat
Copy link
Contributor Author

faddat commented Aug 22, 2022

This is now amended so that it shouldn't be able to blow things up. (very conservative)

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add comments to what the defualts were

goleveldb.go Outdated
options := &opt.Options{
Filter: filter.NewBloomFilter(10),
OpenFilesCacheCapacity: 8192,
BlockCacheCapacity: opt.GiB,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the default here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

200 on mac, 500 on other.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add comments to say what the normal default was.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolutely.

Sorry for delay on this, been real busy.

goleveldb.go Outdated
Comment on lines 31 to 32
BlockCacheCapacity: opt.GiB, // The default value is 8MiB.
WriteBuffer: 64 * opt.MiB, // The default value is 4MiB.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are hefty jumps, what is the reasoning?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are way to high and we should delete them and opt to the default then figure out how to proceed instead of throwing random big numbers out there

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be okay with that. The bloom filter should be a pretty big boost by itself. 1 moment please...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@@ -24,7 +25,10 @@ type GoLevelDB struct {
var _ DB = (*GoLevelDB)(nil)

func NewGoLevelDB(name string, dir string) (*GoLevelDB, error) {
return NewGoLevelDBWithOpts(name, dir, nil)
options := &opt.Options{
Filter: filter.NewBloomFilter(10), // by default, goleveldb doesn't use a bloom filter.
Copy link
Member

@tac0turtle tac0turtle Sep 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also disable seek compaction? im not sure how this effects things though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will look it up this week. Maybe we start here, and start to feed things into the iavl benchmarks-- they work now

@JayT106
Copy link
Contributor

JayT106 commented Sep 12, 2022

feel like should benchmark those changes to know the difference and any other impact. Otherwise, we should think about setting options from the SDK side like:
#50

@tac0turtle
Copy link
Member

feel like should benchmark those changes to know the difference and any other impact. Otherwise, we should think about setting options from the SDK side like:
#50

it did reduce compaction by quite a bit but I wasn't able to see the growth in disk space difference.

@faddat
Copy link
Contributor Author

faddat commented Sep 15, 2022

@JayT106 I agree with you, but would like to mention that benchmarks for this library should properly run in iavl.

@tac0turtle tac0turtle enabled auto-merge (squash) November 13, 2022 14:35
@tac0turtle tac0turtle disabled auto-merge November 13, 2022 14:35
@tac0turtle tac0turtle merged commit dd9f801 into cosmos:main Nov 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants