-
Notifications
You must be signed in to change notification settings - Fork 138
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
CBG-3909: use deltas for pv and mv when persisting to the bucket #7096
Changes from 18 commits
4489b79
1d2adc1
f3c6a7d
f784390
75e9574
0e5f537
3d10502
ac139eb
7af5e2b
857ab82
665b76d
da52e6a
547c706
b94c4c8
902462f
addb767
3d3c510
a0a346a
2b63f97
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1018,6 +1018,52 @@ func HexCasToUint64(cas string) uint64 { | |
return binary.LittleEndian.Uint64(casBytes[0:8]) | ||
} | ||
|
||
// HexCasToUint64ForDelta will convert hex cas to uint64 accounting for any stripped zeros in delta calculation | ||
func HexCasToUint64ForDelta(casByte []byte) (uint64, error) { | ||
var decoded []byte | ||
|
||
// as we strip any zeros off the end of the hex value for deltas, the input delta could be odd length | ||
if len(casByte)%2 != 0 { | ||
evenHexLen := make([]byte, len(casByte), len(casByte)+1) | ||
copy(evenHexLen, casByte) | ||
evenHexLen = append(evenHexLen, '0') | ||
casByte = evenHexLen | ||
} | ||
|
||
// create byte array for decoding into | ||
decodedLen := hex.DecodedLen(len(casByte)) | ||
// binary.LittleEndian.Uint64 expects length 8 byte array, if larger we should error, if smaller | ||
// (because of stripped 0's then we should make it length 8). | ||
if decodedLen > 8 { | ||
return 0, fmt.Errorf("corrupt hex value, decoded length larger than expected") | ||
} | ||
if decodedLen < 8 { | ||
// can be less than 8 given we have stripped the 0's for some values, in this case we need to ensure large eniough | ||
decoded = make([]byte, 8) | ||
} else { | ||
decoded = make([]byte, decodedLen) | ||
} | ||
|
||
if _, err := hex.Decode(decoded, casByte); err != nil { | ||
return 0, err | ||
} | ||
res := binary.LittleEndian.Uint64(decoded) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I don't think the max decoded length line is needed, I have removed it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added some safety checks around this area, let me know what you think |
||
return res, nil | ||
} | ||
|
||
// Uint64ToLittleEndianHexAndStripZeros will convert a uint64 type to little endian hex, stripping any zeros off the end | ||
// + stripping 0x from start | ||
func Uint64ToLittleEndianHexAndStripZeros(cas uint64) string { | ||
hexCas := Uint64CASToLittleEndianHex(cas) | ||
|
||
i := len(hexCas) - 1 | ||
for i > 2 && hexCas[i] == '0' { | ||
i-- | ||
} | ||
// strip 0x from start | ||
return string(hexCas[2 : i+1]) | ||
} | ||
|
||
func HexToBase64(s string) ([]byte, error) { | ||
decoded := make([]byte, hex.DecodedLen(len(s))) | ||
if _, err := hex.Decode(decoded, []byte(s)); err != nil { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1735,3 +1735,40 @@ func TestCASToLittleEndianHex(t *testing.T) { | |
littleEndianHex := Uint64CASToLittleEndianHex(casValue) | ||
require.Equal(t, expHexValue, string(littleEndianHex)) | ||
} | ||
|
||
func TestUint64CASToLittleEndianHexAndStripZeros(t *testing.T) { | ||
hexLE := "0x0000000000000000" | ||
u64 := HexCasToUint64(hexLE) | ||
hexLEStripped := Uint64ToLittleEndianHexAndStripZeros(u64) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Running this test locally, the value of hexLEStripped is "000". Is this what we intend? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good spot, fixed this so now that delta is just 0, also added new test case for this |
||
u64Stripped, err := HexCasToUint64ForDelta([]byte(hexLEStripped)) | ||
require.NoError(t, err) | ||
assert.Equal(t, u64, u64Stripped) | ||
|
||
hexLE = "0xffffffffffffffff" | ||
u64 = HexCasToUint64(hexLE) | ||
hexLEStripped = Uint64ToLittleEndianHexAndStripZeros(u64) | ||
u64Stripped, err = HexCasToUint64ForDelta([]byte(hexLEStripped)) | ||
require.NoError(t, err) | ||
assert.Equal(t, u64, u64Stripped) | ||
|
||
hexLE = "0xd123456e789a0bcf" | ||
u64 = HexCasToUint64(hexLE) | ||
hexLEStripped = Uint64ToLittleEndianHexAndStripZeros(u64) | ||
u64Stripped, err = HexCasToUint64ForDelta([]byte(hexLEStripped)) | ||
require.NoError(t, err) | ||
assert.Equal(t, u64, u64Stripped) | ||
|
||
hexLE = "0xd123456e78000000" | ||
u64 = HexCasToUint64(hexLE) | ||
hexLEStripped = Uint64ToLittleEndianHexAndStripZeros(u64) | ||
u64Stripped, err = HexCasToUint64ForDelta([]byte(hexLEStripped)) | ||
require.NoError(t, err) | ||
assert.Equal(t, u64, u64Stripped) | ||
|
||
hexLE = "0xa500000000000000" | ||
u64 = HexCasToUint64(hexLE) | ||
hexLEStripped = Uint64ToLittleEndianHexAndStripZeros(u64) | ||
u64Stripped, err = HexCasToUint64ForDelta([]byte(hexLEStripped)) | ||
require.NoError(t, err) | ||
assert.Equal(t, u64, u64Stripped) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 testTestUint64CASToLittleEndianHexAndStripZeros
for a zero cas value. Removing this if statement makes the test fail with errorencoding/hex: odd length hex string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't suggesting to remove the if - just to replace
with
as that shouldn't trigger any new allocations when casByte starts out with an odd length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh apologies, completely got wrong end of the stick sorry. Have updated to make this change.