CBG-3909: use deltas for pv and mv when persisting to the bucket #7096

gregns1 · 2024-08-29T14:32:32Z

CBG-3909

Adds functionality to have previous versions and merge version in the persisted hlv stored as deltas.
Added functionality to read the persisted form of hlv and convert the pv and mv lists back to full values from deltas and back into map form in memory
Functionality to take in memory hlv and convert the pv and mv maps into delta lists and persist to the bucket in that form
Currently I have left the fact we have to convert between cas string and uin64 cas a bit back and forth for converting deltas for work in separate ticket (if we decide to change the in memory format back to uint674 for cas)

Pre-review checklist

Removed debug logging (fmt.Print, log.Print, ...)
Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Integration Tests

GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGateway-Integration/2739/

base/util.go

adamcfraser · 2024-09-11T22:40:22Z

base/util.go

+	if err != nil {
+		return 0, err
+	}
+	res := binary.LittleEndian.Uint64(decoded)


Doesn't binary.LittleEndian.Uint64 expect a length 8 []byte as input? Here it looks like we're setting decoded to MaxDecodedLength (18) - can you help me understand the case where casByte[2:] is longer than 8 bytes but we need to support this and are intentionally ignoring trailing bytes?

I thought this was just handling the case where it's less than 8 bytes because we've trimmed trailing zeros.

Good point. I don't think the max decoded length line is needed, I have removed it.

Well, hex.Decode is going to panic if casByte[2:] overflows decoded, so it's possible the previous code was defensive. https://go.dev/play/p/_LEScaOT1FG
Our HexCasToUint64 has an explicit length check to protect against this. Possibly we just want to do the same (but also supporting the case here that the length can be less than 8): doing something like return error if len(casByte) > 10 at the top of the function?

Added some safety checks around this area, let me know what you think

base/util.go

db/document.go

base/util_test.go

db/hybrid_logical_vector.go

adamcfraser · 2024-09-12T14:18:23Z

base/util.go

+	if err != nil {
+		return 0, err
+	}
+	res := binary.LittleEndian.Uint64(decoded)


Well, hex.Decode is going to panic if casByte[2:] overflows decoded, so it's possible the previous code was defensive. https://go.dev/play/p/_LEScaOT1FG
Our HexCasToUint64 has an explicit length check to protect against this. Possibly we just want to do the same (but also supporting the case here that the length can be less than 8): doing something like return error if len(casByte) > 10 at the top of the function?

adamcfraser

Left a suggestion around implementation - have a look and let me know what you think.

db/hybrid_logical_vector.go

Signed-off-by: Gregory Newman-Smith <[email protected]>

adamcfraser · 2024-10-07T20:39:33Z

base/util.go

+		evenHexLen := make([]byte, len(casByte), len(casByte)+1)
+		copy(evenHexLen, casByte)
+		evenHexLen = append(evenHexLen, '0')
+		casByte = evenHexLen


Given than len(casByte) is odd, I think that casByte=append(casByte, '0') is not going to require golang to increase the capacity of the slice (based on default slice capacities). If that's correct I think we can avoid the GC work of the new allocation you've got here.

Running all my tests, capacity for input to this function is always the same as the length. But the only test I run that crosses into this path of execution is the first call to HexCasToUint64ForDelta inside test TestUint64CASToLittleEndianHexAndStripZeros for a zero cas value. Removing this if statement makes the test fail with error encoding/hex: odd length hex string

I wasn't suggesting to remove the if - just to replace

evenHexLen := make([]byte, len(casByte), len(casByte)+1) copy(evenHexLen, casByte) evenHexLen = append(evenHexLen, '0') casByte = evenHexLen

with

casByte = append(casByte, '0')

as that shouldn't trigger any new allocations when casByte starts out with an odd length.

Oh apologies, completely got wrong end of the stick sorry. Have updated to make this change.

adamcfraser · 2024-10-07T21:06:10Z

base/util_test.go

+func TestUint64CASToLittleEndianHexAndStripZeros(t *testing.T) {
+	hexLE := "0x0000000000000000"
+	u64 := HexCasToUint64(hexLE)
+	hexLEStripped := Uint64ToLittleEndianHexAndStripZeros(u64)


Running this test locally, the value of hexLEStripped is "000". Is this what we intend?
It looks like any hex value with only two non-zero digits retains an extra zero. (e.g. 0xa500000000000000 results in a50 when I would have expected a5. Is there a requirement for a minimum length of three?

Good spot, fixed this so now that delta is just 0, also added new test case for this

adamcfraser · 2024-10-07T21:17:11Z

db/hybrid_logical_vector.go

+
+func (vde VersionsDeltas) Less(i, j int) bool {
+	if vde[i].Value == vde[j].Value {
+		return vde[i].SourceID < vde[j].SourceID


For the purpose of computing deltas, does the ordering of sources with matching values matter? i.e. do we actually need SourceID to be acting as a tiebreaker, or can we just stick to vde[i].Value < vde[j].Value below and have equality return Less() = false?

Also, do we have unit test coverage for matching Values?

I think I did this to align with xdcr approach on it. I have removed it because the more I think about it we don't need to align with them on this part. I have returned false in that scenario.

Added a new test case to TestVersionDeltaCalculation for pv map with two entries that have the same value, leading to 0 deltas being computed.

adamcfraser · 2024-10-07T21:30:18Z

db/hybrid_logical_vector.go

+	for src, vrs := range versions {
+		vdm = append(vdm, CreateVersion(src, vrs))
+	}
+


Should we return early here when len(versions) == 1? And do we have test coverage for that scenario?

Good idea, yes we have a scenario where we have single entry in PV in TestPvDeltaReadAndWrite

db/hybrid_logical_vector.go

adamcfraser

Everything looks good except for one clarification on my previous comment (reducing allocations when converting casByte to an even length)

adamcfraser · 2024-10-09T06:02:51Z

base/util.go

+		evenHexLen := make([]byte, len(casByte), len(casByte)+1)
+		copy(evenHexLen, casByte)
+		evenHexLen = append(evenHexLen, '0')
+		casByte = evenHexLen


I wasn't suggesting to remove the if - just to replace

evenHexLen := make([]byte, len(casByte), len(casByte)+1) copy(evenHexLen, casByte) evenHexLen = append(evenHexLen, '0') casByte = evenHexLen

with

casByte = append(casByte, '0')

as that shouldn't trigger any new allocations when casByte starts out with an odd length.

gregns1 assigned gregns1 and adamcfraser and unassigned gregns1 Aug 29, 2024

gregns1 requested a review from adamcfraser September 2, 2024 14:30

gregns1 assigned gregns1 and adamcfraser and unassigned adamcfraser and gregns1 Sep 4, 2024

adamcfraser requested changes Sep 11, 2024

View reviewed changes

adamcfraser assigned gregns1 and unassigned adamcfraser Sep 11, 2024

gregns1 force-pushed the CBG-3909 branch from 7dc7bb5 to ea5a5a4 Compare September 12, 2024 10:44

gregns1 assigned adamcfraser and unassigned gregns1 Sep 12, 2024

adamcfraser requested changes Sep 12, 2024

View reviewed changes

adamcfraser assigned gregns1 and unassigned adamcfraser Sep 12, 2024

gregns1 force-pushed the CBG-3909 branch from 02a4b47 to e6b842d Compare September 13, 2024 11:10

gregns1 assigned adamcfraser and gregns1 and unassigned gregns1 and adamcfraser Sep 13, 2024

adamcfraser requested changes Sep 18, 2024

View reviewed changes

db/hybrid_logical_vector.go Outdated Show resolved Hide resolved

adamcfraser assigned gregns1 and adamcfraser and unassigned adamcfraser and gregns1 Sep 18, 2024

adamcfraser requested changes Sep 24, 2024

View reviewed changes

db/hybrid_logical_vector.go Outdated Show resolved Hide resolved

gregns1 added 10 commits October 3, 2024 13:01

clean up

3d10502

fix from rebase

ac139eb

safety for decoding delta value

7af5e2b

lint error

857ab82

Signed-off-by: Gregory Newman-Smith <[email protected]>

updates to remove leading 0x in deltas

665b76d

fix comment

da52e6a

custom marahal/unmarahal

547c706

lint fix

b94c4c8

lint

902462f

updates to marshal and unmarshal functions

addb767

gregns1 force-pushed the CBG-3909 branch from 7e61223 to addb767 Compare October 4, 2024 10:43

remove unused param

3d3c510

torcolvin assigned gregns1 Oct 4, 2024

adamcfraser assigned adamcfraser and unassigned gregns1 Oct 4, 2024

adamcfraser requested changes Oct 7, 2024

View reviewed changes

adamcfraser assigned gregns1 and unassigned adamcfraser Oct 7, 2024

updated to add new test cases and small changes

a0a346a

gregns1 assigned adamcfraser and unassigned gregns1 Oct 8, 2024

adamcfraser requested changes Oct 9, 2024

View reviewed changes

adamcfraser assigned gregns1 and unassigned adamcfraser Oct 9, 2024

small update

2b63f97

gregns1 assigned adamcfraser and unassigned gregns1 Oct 10, 2024

adamcfraser approved these changes Oct 11, 2024

View reviewed changes

adamcfraser merged commit 7ad9dbf into release/anemone Oct 11, 2024
36 checks passed

adamcfraser deleted the CBG-3909 branch October 11, 2024 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CBG-3909: use deltas for pv and mv when persisting to the bucket #7096

CBG-3909: use deltas for pv and mv when persisting to the bucket #7096

gregns1 commented Aug 29, 2024 •

edited

Loading

adamcfraser Sep 11, 2024

gregns1 Sep 12, 2024

adamcfraser Sep 12, 2024

gregns1 Sep 13, 2024

adamcfraser Sep 12, 2024

adamcfraser left a comment

adamcfraser Oct 7, 2024

gregns1 Oct 8, 2024

adamcfraser Oct 9, 2024

gregns1 Oct 10, 2024

adamcfraser Oct 7, 2024

gregns1 Oct 8, 2024 •

edited

Loading

adamcfraser Oct 7, 2024

adamcfraser Oct 7, 2024

gregns1 Oct 8, 2024

adamcfraser Oct 7, 2024

gregns1 Oct 8, 2024 •

edited

Loading

adamcfraser left a comment

adamcfraser Oct 9, 2024

CBG-3909: use deltas for pv and mv when persisting to the bucket #7096

CBG-3909: use deltas for pv and mv when persisting to the bucket #7096

Conversation

gregns1 commented Aug 29, 2024 • edited Loading

Pre-review checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamcfraser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregns1 Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregns1 Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

adamcfraser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregns1 commented Aug 29, 2024 •

edited

Loading

gregns1 Oct 8, 2024 •

edited

Loading

gregns1 Oct 8, 2024 •

edited

Loading