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

Introduce block cache in badger #1066

Merged
merged 28 commits into from Oct 21, 2019

Conversation

@jarifibrahim
Copy link
Member

jarifibrahim commented Oct 4, 2019

This PR adds a block-level cache to badger.

name             master.txt   only-compression.txt  cache-compression-only.txt  only-encryption.txt  cache-encryption-only.txt  compression-encryption.txt  cache-compression-encryption.txt
Read-16          83.1ms ± 1%          364.1ms ± 3%                396.7ms ± 2%         344.6ms ± 9%               358.6ms ± 5%                434.1ms ± 2%                      471.4ms ± 7%
ReadAndBuild-16   823ms ± 3%           3968ms ± 1%                 4199ms ± 1%          1906ms ± 1%                2057ms ± 3%                 5058ms ± 1%                       5243ms ± 1%
ReadMerged-16     848ms ± 1%           1065ms ± 1%                 1183ms ± 3%          1026ms ± 2%                1123ms ± 7%                 1137ms ± 1%                       1246ms ± 5%
RandomRead-16    2.02µs ± 1%          23.66µs ± 3%                 4.62µs ± 5%         17.65µs ± 1%                4.44µs ± 4%                28.82µs ± 3%                       5.10µs ± 8%

This change is Reviewable

@jarifibrahim jarifibrahim requested review from ashish-goswami, manishrjain and dgraph-io/team as code owners Oct 4, 2019
Copy link

pullrequest bot left a comment

A review job has been created and sent to the PullRequest network.


@jarifibrahim you can click here to see the review status or cancel the code review job.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 4, 2019

Coverage Status

Coverage increased (+0.09%) to 77.535% when pulling 0af9b63 on ibrahim/cache into 385da91 on master.

Copy link

pullrequest bot left a comment

Provided some follow-up to my initial review focusing on readability.


Reviewed with ❤️ by PullRequest

rand.Seed(time.Now().Unix())
opts := Options{Compression: options.ZSTD, BlockSize: 4 * 1024, BloomFalsePositive: 0.01}
if cache == nil {
var err error
cache, err = ristretto.NewCache(&ristretto.Config{

This comment has been minimized.

Copy link
@pullrequest

pullrequest bot Oct 5, 2019

This is likely classified as a nit; however I think it has readability implications. ristretto.NewCache accepts a pointer value for its Config. There is limited (readability) value initializing the struct then immediately taking its pointer value. I think it is a two step process and should be written with that in mind for enhanced readability.

@@ -348,7 +351,14 @@ func (t *Table) block(idx int) (*block, error) {
if idx >= len(t.blockIndex) {
return nil, errors.New("block out of index")
}

var blkKey string

This comment has been minimized.

Copy link
@pullrequest

pullrequest bot Oct 5, 2019

I know you mean block key, but I think it may also be interpreted as black key as blk is also used as short hand for the color, I do not see any harm in blockKey as an identifier.

Copy link
Contributor

ashish-goswami left a comment

Changes looks good. Got minor comments.

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)


options.go, line 93 at r2 (raw file):

	maxBatchSize  int64 // max batch size in bytes

	cache *ristretto.Cache

should we call this as blockCache as it is caching blocks of tables. For now we are just caching table blocks in future we can cache some other things.


table/table_test.go, line 758 at r2 (raw file):

	n := int(5 * 1e6)

	var cache, _ = ristretto.NewCache(&ristretto.Config{

This change makes cache compulsory in benchmarks, we should have both with cache and without cache benchmarks.


table/table_test.go, line 792 at r2 (raw file):

	var tables []*Table

	var cache, err = ristretto.NewCache(&ristretto.Config{

same here.

jarifibrahim added 2 commits Oct 9, 2019
Copy link
Member Author

jarifibrahim left a comment

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @ashish-goswami, @manishrjain, and @pullrequest[bot])


options.go, line 93 at r2 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

should we call this as blockCache as it is caching blocks of tables. For now we are just caching table blocks in future we can cache some other things.

Sure. Renamed it to blockCache.


table/table.go, line 354 at r1 (raw file):

Previously, pullrequest[bot] wrote…

I know you mean block key, but I think it may also be interpreted as black key as blk is also used as short hand for the color, I do not see any harm in blockKey as an identifier.

BlockKey could be ambiguous since we have keys in a block and the first key could be called blockKey. I've renamed it to cacheKey


table/table_test.go, line 891 at r1 (raw file):

Previously, pullrequest[bot] wrote…

This is likely classified as a nit; however I think it has readability implications. ristretto.NewCache accepts a pointer value for its Config. There is limited (readability) value initializing the struct then immediately taking its pointer value. I think it is a two step process and should be written with that in mind for enhanced readability.

Yeah. Fixed this by moving the config to a global variable. We need the same config at multiple places.


table/table_test.go, line 758 at r2 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

This change makes cache compulsory in benchmarks, we should have both with cache and without cache benchmarks.

Discussed this with @ashish-goswami and we decided to the benchmarks as it is.


table/table_test.go, line 792 at r2 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

same here.

Discussed this with @ashish-goswami and we decided to the benchmarks as it is.

Copy link
Member

manishrjain left a comment

Looks good. Got few comments. Defer to Shekar and Ashish.

Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, and @pullrequest[bot])


table/table.go, line 357 at r3 (raw file):

	if t.opt.Cache != nil {
		cacheKey = t.blockCacheKey(idx)
		cachedBlk, ok := t.opt.Cache.Get(cacheKey)

blk, ok :=


table/table.go, line 421 at r3 (raw file):

	}
	if t.opt.Cache != nil {
		t.opt.Cache.Set(cacheKey, blk, 1)

Allow users to specify how many bytes to use for cache. And then use the cap of the block byte slice to determine the cost.


table/table.go, line 427 at r3 (raw file):

func (t *Table) blockCacheKey(idx int) string {
	return fmt.Sprintf("%d_%d", t.ID(), idx)

fmt.Sprintf considered harmful.

Make an uint64 or something cheap. Do not use strings. Also avoid the cost of hash.

(uint64(uint32(t.ID()))) << 32 | (uint64(idx) & 0xffffffff)

Add a check that t.ID() or block id does not exceed uint32 = 4GB - 1.

Copy link
Member Author

jarifibrahim left a comment

Reviewable status: 1 of 7 files reviewed, 8 unresolved discussions (waiting on @ashish-goswami, @manishrjain, and @pullrequest[bot])


table/table.go, line 357 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

blk, ok :=

Done.


table/table.go, line 421 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Allow users to specify how many bytes to use for cache. And then use the cap of the block byte slice to determine the cost.

Done.


table/table.go, line 427 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

fmt.Sprintf considered harmful.

Make an uint64 or something cheap. Do not use strings. Also avoid the cost of hash.

(uint64(uint32(t.ID()))) << 32 | (uint64(idx) & 0xffffffff)

Add a check that t.ID() or block id does not exceed uint32 = 4GB - 1.

Done.

Copy link
Contributor

ashish-goswami left a comment

:lgtm:

Reviewable status: 1 of 6 files reviewed, 5 unresolved discussions (waiting on @manishrjain and @pullrequest[bot])

Copy link
Member Author

jarifibrahim left a comment

Reviewable status: 1 of 6 files reviewed, 7 unresolved discussions (waiting on @manishrjain and @pullrequest[bot])


db.go, line 283 at r5 (raw file):

	config := ristretto.Config{
		NumCounters: 5 * opt.MaxCacheSize,

Any value greater than 1 GB for maxCacheSize and numCounters greater than 5 * maxCacheSize causes OOM.


options.go, line 122 at r5 (raw file):

		VerifyValueChecksum:     false,
		Compression:             options.ZSTD,
		MaxCacheSize:            1 << 30, // 1 GB

Any value greater than 1 GB for maxCacheSize and numCounters greater than 5 * maxCacheSize causes OOM.

Copy link

shekarm left a comment

Reviewable status: 1 of 10 files reviewed, 6 unresolved discussions (waiting on @jarifibrahim, @manishrjain, and @pullrequest[bot])


table/table_test.go, line 51 at r6 (raw file):

		Compression:        options.ZSTD,
		LoadingMode:        options.LoadToRAM,
		BlockSize:          4 * 1024,

Should we set the block size to be the same as the block size of the underlying file system? Would that give any performance advantage in reading/writing from fs?

Copy link
Member

manishrjain left a comment

:lgtm: Changes as I mentioned. Rest is good to go.

Reviewed 2 of 5 files at r4, 1 of 3 files at r5, 6 of 6 files at r6.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @jarifibrahim, @manishrjain, and @pullrequest[bot])


db.go, line 282 at r6 (raw file):

	}

	config := ristretto.Config{

Please add a comment about changing block size.


db.go, line 283 at r6 (raw file):

	config := ristretto.Config{
		NumCounters: 10 * (opt.MaxCacheSize / int64(opt.BlockSize)),

opt.MaxCacheSize * 0.05 * 2

5% of the memory is being used for NumCounters. We can store 2 counters per byte.


db.go, line 284 at r6 (raw file):

	config := ristretto.Config{
		NumCounters: 10 * (opt.MaxCacheSize / int64(opt.BlockSize)),
		MaxCost:     opt.MaxCacheSize,

Perhaps *0.95


db.go, line 286 at r6 (raw file):

		MaxCost:     opt.MaxCacheSize,
		BufferItems: 64,
		Metrics:     false,

true. And add a way to get the metrics back.


table/table.go, line 158 at r6 (raw file):

func (b *block) size() int64 {
	return int64(3*int(unsafe.Sizeof(int(0))) /* Size of offset, entriesIndexStart and chkLen */ +

For unsafe.Sizeof(int(0))

So, make it a variable, so it can be calculated during runtime once. And use that for all the calls to size().

Copy link

pullrequest bot left a comment

Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard

@jarifibrahim jarifibrahim mentioned this pull request Oct 21, 2019
Copy link
Member Author

jarifibrahim left a comment

Dismissed @manishrjain and @pullrequest[bot] from 3 discussions.
Reviewable status: 2 of 12 files reviewed, 6 unresolved discussions (waiting on @manishrjain and @shekarm)


db.go, line 282 at r6 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Please add a comment about changing block size.

Added a comment in options.go file


db.go, line 283 at r6 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

opt.MaxCacheSize * 0.05 * 2

5% of the memory is being used for NumCounters. We can store 2 counters per byte.

Done.


db.go, line 284 at r6 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Perhaps *0.95

Done.


db.go, line 286 at r6 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

true. And add a way to get the metrics back.

Enabling metrics caused race condition see dgraph-io/ristretto#92 . I'll enable this once the issue with ristretto is resolved.


table/table.go, line 158 at r6 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

For unsafe.Sizeof(int(0))

So, make it a variable, so it can be calculated during runtime once. And use that for all the calls to size().

Added it to constants at the top. The unsafe.SizeOf(..) will be evaluated at compile time.


table/table_test.go, line 51 at r6 (raw file):

Previously, shekarm (Shekar Mantha) wrote…

Should we set the block size to be the same as the block size of the underlying file system? Would that give any performance advantage in reading/writing from fs?

Yes. as discussed @shekarm will figure out how to determine the block size at run time. This will be changed in a separate PR.

Copy link
Member Author

jarifibrahim left a comment

Dismissed @manishrjain and @shekarm from 6 discussions.
Reviewable status: 2 of 12 files reviewed, all discussions resolved (waiting on @manishrjain)

@jarifibrahim jarifibrahim dismissed stale reviews from manishrjain and shekarm Oct 21, 2019

Addressed all comments

@jarifibrahim jarifibrahim merged commit b9056f1 into master Oct 21, 2019
5 of 8 checks passed
5 of 8 checks passed
code-review/reviewable 10 files left (manishrjain)
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
GolangCI No issues found!
Details
Unit Tests (badger) TeamCity build finished
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@jarifibrahim jarifibrahim deleted the ibrahim/cache branch Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.