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

#96 Fixes memory leak due to Oct 1st regression in processItems #99

Merged
merged 1 commit into from
Nov 6, 2019

Conversation

kevburnsjr
Copy link
Contributor

@kevburnsjr kevburnsjr commented Nov 5, 2019

See #96


I ran the proof with two different versions of ristretto and got 2 different results.
This is definitely a regression.

Found the offending commit
d963fa2

$ go get github.com/dgraph-io/ristretto@d963fa241740c4012b003362a7602e6ae764b104
go: finding github.com/dgraph-io/ristretto d963fa241740c4012b003362a7602e6ae764b104
go: downloading github.com/dgraph-io/ristretto v0.0.0-20191001142246-d963fa241740
go: extracting github.com/dgraph-io/ristretto v0.0.0-20191001142246-d963fa241740

$ go run ristretto-bug.go
time: 1s alloc: 1034.71 MiB hits: 75 misses: 3747
time: 2s alloc: 1043.73 MiB hits: 117 misses: 4378
time: 3s alloc: 1050.75 MiB hits: 109 misses: 4374
time: 4s alloc: 1076.77 MiB hits: 138 misses: 4339
time: 5s alloc: 1126.79 MiB hits: 178 misses: 4337

$ go get github.com/dgraph-io/ristretto@7028ca5adaaeebef6f8498040b8a77189a4387b2
go: finding github.com/dgraph-io/ristretto 7028ca5adaaeebef6f8498040b8a77189a4387b2
go: downloading github.com/dgraph-io/ristretto v0.0.0-20190930223733-7028ca5adaae
go: extracting github.com/dgraph-io/ristretto v0.0.0-20190930223733-7028ca5adaae

$ go run ristretto-bug.go
time: 1s alloc: 1040.71 MiB hits: 78 misses: 3807
time: 2s alloc: 1031.72 MiB hits: 105 misses: 4403
time: 3s alloc: 1030.73 MiB hits: 110 misses: 4373
time: 4s alloc: 1036.74 MiB hits: 114 misses: 4335
time: 5s alloc: 1029.75 MiB hits: 107 misses: 4370

Reading the diff I saw a logic change that looked like an error since policy.Add can, in some cases, return victims when added is false.
https://github.com/dgraph-io/ristretto/blob/master/policy.go#L168

// Before Oct 1st

victims, added := c.policy.Add(item.key, item.cost)
if added {
	// ...
}
for _, victim := range victims {
	// ...
}
// After Oct 1st commit

if victims, added := c.policy.Add(item.key, item.cost); added {
	// ...
	for _, victim := range victims {
		// ...
	}
}

This commit extracts the for loop from the if statement, fixing the memory leak.


This change is Reviewable

@kevburnsjr kevburnsjr changed the title #19 Fixes memory leak due to Oct 1st regression in processItems #96 Fixes memory leak due to Oct 1st regression in processItems Nov 5, 2019
Copy link
Contributor

@karlmcguire karlmcguire left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jarifibrahim and @manishrjain)

@karlmcguire
Copy link
Contributor

Thanks @kevburnsjr.

@karlmcguire karlmcguire merged commit 1dd5a4d into dgraph-io:master Nov 6, 2019
@kevburnsjr kevburnsjr deleted the 19-mem-leak branch November 6, 2019 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants