Introduce block cache in badger #1066

jarifibrahim · 2019-10-04T16:22:24Z

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

pullrequest

✅ 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 · 2019-10-04T17:02:22Z

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

pullrequest

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

Reviewed with ❤️ by PullRequest

pullrequest · 2019-10-05T03:05:33Z

table/table_test.go

 	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 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.

pullrequest · 2019-10-05T03:05:33Z

table/table.go

@@ -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


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.

ashish-goswami

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

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.

manishrjain

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.

jarifibrahim

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.

ashish-goswami

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

jarifibrahim

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.

shekarm

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?

manishrjain

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().

pullrequest

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

jarifibrahim

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 hypermodeinc/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.

jarifibrahim

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

Addressed all comments

This commit adds ristretto to badger. All the reads are done through the block cache. The block cache holds blocks in a decompressed and unencrypted state. The memory usage of cache can be changed by `MaxCacheSize` option. The default size of the cache is 1 GB.

This commit adds ristretto to badger. All the reads are done through the block cache. The block cache holds blocks in a decompressed and unencrypted state. The memory usage of cache can be changed by `MaxCacheSize` option. The default size of the cache is 1 GB. Co-authored-by: Ibrahim Jarif <jarifibrahim@gmail.com>

Ibrahim Jarif added 3 commits October 3, 2019 19:37

Introduce cache in badger

7e9e2f4

Add cache to table benchmarks

a563093

Remove unused code

38e0db5

jarifibrahim requested review from ashish-goswami and manishrjain as code owners October 4, 2019 16:22

jarifibrahim requested a review from a team October 4, 2019 16:22

pullrequest bot reviewed Oct 4, 2019

View reviewed changes

pullrequest bot reviewed Oct 5, 2019

View reviewed changes

fix broken test

99cd704

ashish-goswami suggested changes Oct 7, 2019

View reviewed changes

Ibrahim Jarif added 2 commits October 9, 2019 15:22

Address review comments

1567943

Address review comments

61f69c4

jarifibrahim commented Oct 9, 2019

View reviewed changes

manishrjain approved these changes Oct 14, 2019

View reviewed changes

Ibrahim Jarif added 5 commits October 15, 2019 16:13

Enable ristretto in badger

ce54224

fix nil pointer

84cf3d9

Use cap instead of len

212506c

fixup

6204da2

Merge branch 'master' into ibrahim/cache

2efde81

jarifibrahim commented Oct 15, 2019

View reviewed changes

fixup

0f66a61

ashish-goswami approved these changes Oct 15, 2019

View reviewed changes

Ibrahim Jarif added 2 commits October 15, 2019 19:41

fixup

ac80321

Reduce cache size and numcounters to prevent OOM

b33ad7f

jarifibrahim commented Oct 15, 2019

View reviewed changes

Ibrahim Jarif added 4 commits October 16, 2019 10:29

Update ristretto to latest version

6f4db80

fixup

516a4c4

Close cache on db.close()

aa4e4c1

Clear cache on dropAll

332eaab

Ibrahim Jarif added 3 commits October 16, 2019 17:12

set cache size to 2 GB

28ad833

fixup

e31443a

TestGoroutineLeak is now more reliable

f04cd93

shekarm previously requested changes Oct 18, 2019

View reviewed changes

manishrjain previously requested changes Oct 18, 2019

View reviewed changes

pullrequest bot reviewed Oct 19, 2019

View reviewed changes

Ibrahim Jarif added 3 commits October 21, 2019 12:49

Address review comments

2abe246

Reduce default size of cache for builds to work

eac4cc6

Merge branch 'master' into ibrahim/cache

2d60042

jarifibrahim mentioned this pull request Oct 21, 2019

Race in cache.clear hypermodeinc/ristretto#92

Closed

Ibrahim Jarif added 4 commits October 21, 2019 14:35

Use testOptions instead of default options

1dee4ae

Disable ristretto metrics

f4f35e3

Disable metrics

c1ff620

Retrigger licence check

0af9b63

jarifibrahim commented Oct 21, 2019

View reviewed changes

jarifibrahim merged commit b9056f1 into master Oct 21, 2019

jarifibrahim deleted the ibrahim/cache branch October 21, 2019 10:49

danielmai mentioned this pull request Jan 3, 2020

Introduce LRU cache in Badger #663

Closed

zhiqiangxu mentioned this pull request Mar 8, 2020

Does badger cache guarantee data consistency? #1252

Closed

Introduce block cache in badger #1066

Introduce block cache in badger #1066

Uh oh!

Conversation

jarifibrahim commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pullrequest bot left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pullrequest bot left a comment

Choose a reason for hiding this comment

Uh oh!

pullrequest bot Oct 5, 2019

Choose a reason for hiding this comment

Uh oh!

pullrequest bot Oct 5, 2019

Choose a reason for hiding this comment

Uh oh!

ashish-goswami left a comment

Choose a reason for hiding this comment

Uh oh!

jarifibrahim left a comment

Choose a reason for hiding this comment

Uh oh!

manishrjain left a comment

Choose a reason for hiding this comment

Uh oh!

jarifibrahim left a comment

Choose a reason for hiding this comment

Uh oh!

ashish-goswami left a comment

Choose a reason for hiding this comment

Uh oh!

jarifibrahim left a comment

Choose a reason for hiding this comment

Uh oh!

shekarm left a comment

Choose a reason for hiding this comment

Uh oh!

manishrjain left a comment

Choose a reason for hiding this comment

Uh oh!

pullrequest bot left a comment

Choose a reason for hiding this comment

Uh oh!

jarifibrahim left a comment

Choose a reason for hiding this comment

Uh oh!

jarifibrahim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jarifibrahim commented Oct 4, 2019 •

edited

Loading

coveralls commented Oct 4, 2019 •

edited

Loading