feat(vlog): making vlog threshold dynamic 6ce3b7c #1635

aman-bansal · 2021-01-05T15:09:51Z

I ran this for 200M entries. Value threshold is set to 128 initially with max of 2048 and threshold range of 128-1024.
With Dynamic Threshold set to true it takes 52m. Vlog file size were around 4Gi. Threshold sets to around 770 after certain iterations.

With Dynamic Threshold set to false, it takes around 35 min for the write bench to write the data. vlog file size in this case is around 70Gi

Percentage distribution of value sizes were 80% less than 512B, 19% 512B-768B, 0.99% 768-1024B, and 0.01% 1024-2048 B.

This change is

CLAassistant · 2021-01-05T15:09:56Z

All committers have signed the CLA.

…dger into aman/vlog_threshold_dynamic

jarifibrahim · 2021-01-27T13:20:09Z

@aman-bansal can you please resolve the conflicts?

NamanJain8

Great @aman-bansal . Got few comments. Also, we are considering the vlog rewrites but not the keys which are deleted while compaction. Should be okay I believe.

NamanJain8 · 2021-01-27T19:15:05Z

structs.go

+	if v < e.valThreshold {
+		return k + v + 2 // Meta, UserMeta
+	}
+	return k + 12 + 2


Use vPtrSize instead of 12. Also, please add back the comment.

txn.go

value.go

NamanJain8 · 2021-01-27T19:41:50Z

value.go

+		case <-v.closer.HasBeenClosed():
+			return
+		case val := <-v.valueCh:
+			if val == nil {


When will this be nil?

safe check only. No use case per se.

value.go

jarifibrahim

Got some comments

Reviewed 1 of 16 files at r4.
Reviewable status: 1 of 18 files reviewed, 12 unresolved discussions (waiting on @aman-bansal, @jarifibrahim, @manishrjain, and @NamanJain8)

options.go, line 62 at r8 (raw file):

	VLogPercentile    float64
	MinValueThreshold int64

This is a breaking change. This PR doesn't need to be a breaking change.

options.go, line 348 at r8 (raw file):

// [MinValueThreshold, Options.maxValueThreshold]
//
// Say VLogPercentile with 1.0 means threshold will eventually sets too Options.maxValueThreshold

will be eventually set to ...

structs.go, line 172 at r8 (raw file):

func (e *Entry) skipVlog(threshold int64) bool {
	if e.valThreshold == 0 {
		e.valThreshold = threshold

When would valThreshold be zero? This function doesn't need to change the value of the valThreshold.

structs.go, line 174 at r8 (raw file):

		e.valThreshold = threshold
	}
	if int64(len(e.Value)) < e.valThreshold {

return int64(len(e.Value)) < e.valThreshold

value.go, line 1164 at r7 (raw file):

Previously, aman-bansal (aman bansal) wrote…

safe check only. No use case per se.

If we don't expect this to be nil, I'd at least log it.

value.go, line 892 at r8 (raw file):

		vlog.numEntriesWritten += uint32(written)
		vlog.db.threshold.update(valueSizes)

@aman-bansal do you need to update the sizes one txn at a time? Can't we do this outside both the for loops? One single call to threshold.update(...)

value.go, line 1175 at r8 (raw file):

			p := int64(v.vlMetrics.Percentile(v.percentile))
			if atomic.LoadInt64(&v.valueThreshold) != p {
				// v.opt.Infof("updating value threshold from: %d to: %d", vt, p)

stale comment.

jarifibrahim

Reset the histogram on drop all otherwise LGTM.

manishrjain

Hey @aman-bansal , what's the status here?

Reviewable status: 1 of 18 files reviewed, 13 unresolved discussions (waiting on @aman-bansal, @jarifibrahim, and @NamanJain8)

db.go, line 119 at r4 (raw file):

	opt.maxBatchSize = (15 * opt.MemTableSize) / 100
	opt.maxBatchCount = opt.maxBatchSize / int64(skl.MaxNodeSize)
	//opt.maxValueThreshold = 1024.0

Rename opt.ValueThreshold to opt.MinValueThreshold. And then this one is max. And do add a comment.

aman-bansal

Reviewable status: 1 of 15 files reviewed, 13 unresolved discussions (waiting on @jarifibrahim, @manishrjain, and @NamanJain8)

db.go, line 119 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Rename opt.ValueThreshold to opt.MinValueThreshold. And then this one is max. And do add a comment.

Done. We have kept the name same because otherwise it would become breaking change. we believe this change doesnt need to be breaking change.

options.go, line 62 at r8 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

This is a breaking change. This PR doesn't need to be a breaking change.

Done.

options.go, line 348 at r8 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

will be eventually set to ...

Done.

structs.go, line 167 at r7 (raw file):

Previously, NamanJain8 (Naman Jain) wrote…

Use vPtrSize instead of 12. Also, please add back the comment.

Done.

structs.go, line 172 at r8 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

When would valThreshold be zero? This function doesn't need to change the value of the valThreshold.

Done.

structs.go, line 174 at r8 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

return int64(len(e.Value)) < e.valThreshold

Done.

txn.go, line 381 at r7 (raw file):

Previously, aman-bansal (aman bansal) wrote…

This check is for inmemory. In this mode, the value threshold will work in a default behaviour. It adds basic validation for that mode i believe.

Done.

value.go, line 72 at r7 (raw file):

Previously, aman-bansal (aman bansal) wrote…

It was looking bit noisy to add int64 everywhere i.e. channels, req tracker etc. I did this so as to add context to the variable. Any thoughts about this?

Done.

value.go, line 845 at r7 (raw file):

Previously, NamanJain8 (Naman Jain) wrote…

This needs to be inside the for loop. Also, we can specify the capacity upfront.
make(ReqValSiz, 0, len(req.Entries))

Done.

value.go, line 1164 at r7 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

If we don't expect this to be nil, I'd at least log it.

Done.

value.go, line 1171 at r7 (raw file):

Previously, aman-bansal (aman bansal) wrote…

yes thanks. It was changed recently. comment was outdated thanks :).

Done.

value.go, line 892 at r8 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

@aman-bansal do you need to update the sizes one txn at a time? Can't we do this outside both the for loops? One single call to threshold.update(...)

Done.

value.go, line 1175 at r8 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

stale comment.

Done.

aman-bansal added 3 commits January 5, 2021 14:17

making vlog threshold dynamic

6ce3b7c

making histogram points fixed

ea50ff4

giving option to set dynamic threshold

dcb5675

aman-bansal added 9 commits January 6, 2021 19:37

refactoring to better handle value threshold

ffe6c4c

name refactor

5a55c1b

making write bench to work with dynamic threshold

db2e47e

fixing test cases

d96f15b

fixing all failing test cases

3712d0e

fixing value log option issue

d55bd9b

moving value threshold to vlog rather than options

312c89a

write benchmark change to test out few things

87908e8

making valthreshold to the txn and req

6d600e3

aman-bansal requested review from jarifibrahim and manishrjain as code owners January 8, 2021 06:01

aman-bansal marked this pull request as ready for review January 8, 2021 06:01

aman-bansal added 13 commits January 8, 2021 11:45

making vlog threshold dynamic

81fcf87

making histogram points fixed

891f2f1

giving option to set dynamic threshold

6a14b49

refactoring to better handle value threshold

03d08f5

name refactor

431aa46

making write bench to work with dynamic threshold

5192d08

fixing test cases

127c692

fixing all failing test cases

a3618f9

fixing value log option issue

28de380

moving value threshold to vlog rather than options

63adb1a

write benchmark change to test out few things

94f08f2

making valthreshold to the txn and req

4fd2c98

test case fix

d16bb34

aman-bansal force-pushed the aman/vlog_threshold_dynamic branch from ac57104 to d16bb34 Compare January 8, 2021 06:23

aman-bansal and others added 6 commits January 11, 2021 11:13

adding val threshold is backup

70d6f40

Merge branch 'aman/vlog_threshold_dynamic' of github.com:dgraph-io/ba…

d728f09

…dger into aman/vlog_threshold_dynamic

go fmt

6939e98

refactor some code

4827d6f

making entries aware of value threshold

980be43

Merge branch 'master' into aman/vlog_threshold_dynamic

8d6887f

aman-bansal force-pushed the aman/vlog_threshold_dynamic branch from 85bceb1 to 8d6887f Compare January 21, 2021 06:56

aman-bansal added 4 commits January 21, 2021 13:42

adding comment for the use of entry

255147a

removing dynamic code from write bench

2230255

making vlog threshold in entry to manage dynamic values

9ad48c2

removing redundant code

3da6b0f

aman-bansal added 2 commits January 27, 2021 19:08

Merge branch 'master' into aman/vlog_threshold_dynamic

5ce8647

updating ristretto

54d5591

aman-bansal force-pushed the aman/vlog_threshold_dynamic branch from cb2e7ad to 54d5591 Compare January 27, 2021 13:54

fixing test

b2a80ee

NamanJain8 reviewed Jan 27, 2021

View reviewed changes

addressing review comments

9d1a18d

jarifibrahim reviewed Jan 28, 2021

View reviewed changes

jarifibrahim approved these changes Jan 28, 2021

View reviewed changes

manishrjain reviewed Feb 3, 2021

View reviewed changes

aman-bansal added 2 commits February 4, 2021 17:19

fixing review comments with clearing metrics when called drop all

7cabc21

fixing write bench to original

b63fa45

aman-bansal commented Feb 4, 2021

View reviewed changes

aman-bansal merged commit 5dbae10 into master Feb 4, 2021

aman-bansal deleted the aman/vlog_threshold_dynamic branch February 4, 2021 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(vlog): making vlog threshold dynamic 6ce3b7c #1635

feat(vlog): making vlog threshold dynamic 6ce3b7c #1635

aman-bansal commented Jan 5, 2021 •

edited

Loading

CLAassistant commented Jan 5, 2021 •

edited

Loading

jarifibrahim commented Jan 27, 2021

NamanJain8 left a comment

NamanJain8 Jan 27, 2021

NamanJain8 Jan 27, 2021

aman-bansal Jan 27, 2021

jarifibrahim left a comment

jarifibrahim left a comment

manishrjain left a comment

aman-bansal left a comment

feat(vlog): making vlog threshold dynamic 6ce3b7c #1635

feat(vlog): making vlog threshold dynamic 6ce3b7c #1635

Conversation

aman-bansal commented Jan 5, 2021 • edited Loading

CLAassistant commented Jan 5, 2021 • edited Loading

jarifibrahim commented Jan 27, 2021

NamanJain8 left a comment

Choose a reason for hiding this comment

NamanJain8 Jan 27, 2021

Choose a reason for hiding this comment

NamanJain8 Jan 27, 2021

Choose a reason for hiding this comment

aman-bansal Jan 27, 2021

Choose a reason for hiding this comment

jarifibrahim left a comment

Choose a reason for hiding this comment

jarifibrahim left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

aman-bansal left a comment

Choose a reason for hiding this comment

aman-bansal commented Jan 5, 2021 •

edited

Loading

CLAassistant commented Jan 5, 2021 •

edited

Loading