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

BAAS-32412: add nil check in mem usage for key and item in builtin_set #124

Merged
merged 2 commits into from
Jun 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion builtin_map_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ func createOrderedMap(vm *Runtime, size int) *orderedMap {
value: value,
}
// These iter items are necessary for testing the mem usage
// estimation since that's who we iterate through the map.
// estimation since that's how we iterate through the map.
if i > 0 {
ht[uint64(i)].iterPrev = ht[uint64(i-1)]
ht[uint64(i-1)].iterNext = ht[uint64(i)]
Expand Down
24 changes: 13 additions & 11 deletions builtin_set.go
Original file line number Diff line number Diff line change
Expand Up @@ -364,18 +364,20 @@ func (so *setObject) estimateMemUsage(ctx *MemUsageContext) (estimate uint64, er
}
samplesVisited += 1

// We still want to account for both key and value if we return a non-zero value on error.
// This could otherwise skew the estimate when in reality key/value pairs contribute to
// mem usage together.
Comment on lines -367 to -369
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this on purpose to align with how we normally calculate mem usage, can refactor later if we notice this being an issue but I expect it to be rare.

inc, incErr := item.key.MemUsage(ctx)
memUsage += inc
inc, valErr := item.value.MemUsage(ctx)
memUsage += inc
if valErr != nil {
return computeMemUsageEstimate(memUsage, samplesVisited, totalItems), valErr
if item.key != nil {
inc, err := item.key.MemUsage(ctx)
memUsage += inc
if err != nil {
return computeMemUsageEstimate(memUsage, samplesVisited, totalItems), err
}
}
if incErr != nil {
return computeMemUsageEstimate(memUsage, samplesVisited, totalItems), incErr

if item.value != nil {
inc, err := item.value.MemUsage(ctx)
memUsage += inc
if err != nil {
return computeMemUsageEstimate(memUsage, samplesVisited, totalItems), err
}
}
}

Expand Down
32 changes: 32 additions & 0 deletions builtin_set_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,28 @@ func TestSetHasFloatVsInt(t *testing.T) {
testScript(SCRIPT, valueTrue, t)
}

func createOrderedMapWithNilValues(size int) *orderedMap {
ht := make(map[uint64]*mapEntry, 0)
for i := 0; i < size; i += 1 {
ht[uint64(i)] = &mapEntry{
key: nil,
value: nil,
}
// These iter items are necessary for testing the mem usage
// estimation since that's how we iterate through the map.
if i > 0 {
ht[uint64(i)].iterPrev = ht[uint64(i-1)]
ht[uint64(i-1)].iterNext = ht[uint64(i)]
}
}
return &orderedMap{
size: size,
iterFirst: ht[uint64(0)],
iterLast: ht[uint64(size-1)],
hashTable: ht,
}
}

func TestSetObjectMemUsage(t *testing.T) {
vm := New()

Expand Down Expand Up @@ -274,6 +296,16 @@ func TestSetObjectMemUsage(t *testing.T) {
(5+SizeString)*20,
errExpected: nil,
},
{
name: "mem above estimate threshold and within memory limit and nil values returns correct mem usage",
mu: NewMemUsageContext(vm, 88, 100, 50, 1, 0.1, TestNativeMemUsageChecker{}),
so: &setObject{
m: createOrderedMapWithNilValues(3),
},
// baseObject
expectedMem: SizeEmptyStruct,
errExpected: nil,
},
{
name: "mem is SizeEmptyStruct given a nil orderedMap object",
mu: NewMemUsageContext(vm, 88, 5000, 50, 50, 0.1, TestNativeMemUsageChecker{}),
Expand Down
Loading