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

Potential Memory Leak Due to Slice Expression #2118

Open
Lslightly opened this issue Oct 8, 2024 · 4 comments
Open

Potential Memory Leak Due to Slice Expression #2118

Lslightly opened this issue Oct 8, 2024 · 4 comments

Comments

@Lslightly
Copy link

Dear developers,
I used a self-written code linter to check the project and discovered a potential memory leak issue.

In the following code, before the slice expression db.imm[1:], the elements of db.imm[0] are not set to nil. This could potentially cause the objects pointed to by each element of db.imm[0] to be continuously referenced by the underlying array pointed to by the db.imm slice. As a result, these objects may not be garbage collected in a timely manner by the garbage collector.

badger/db.go

Line 1132 in a9edda1

db.imm = db.imm[1:]

			y.AssertTrue(mt == db.imm[0])
			db.imm = db.imm[1:]
			mt.DecrRef() // Return memory.

In a high-load production environment, if this memory leak persists, it could lead to increased memory consumption and eventually slow down the application or even cause out-of-memory errors.

A possible solution is setting the elements of db.imm[0] to nil before the slice expression. This way, the objects that are no longer needed after the slice expression can be properly garbage collected.

However, I am not sure if this is indeed a problem. I would greatly appreciate it if you can kindly confirm whether setting nil may cause wrong behaviors of the program.

Thank you for your time and efforts in reading this issue and maintaining the project.

@harshil-goel
Copy link
Contributor

So db.imm[0] is mt. As you can see the AssertTrue above it. mt.DecrRef then deletes the data as per requirement. I hope that clarifies your doubt.

@Lslightly
Copy link
Author

Lslightly commented Oct 8, 2024

So db.imm[0] is mt. As you can see the AssertTrue above it. mt.DecrRef then deletes the data as per requirement. I hope that clarifies your doubt.

Oh, thanks for the immediate reply. But the underlying array of db.imm can keep the object mt points to alive although mt won't be accessed. If the size of object mt points to is large enough, there might be memory leak because the object mt points to will be referenced by the underlying array of db.imm . Does db.imm hold many references to objects for a long time? Or compared to the memory size of mt, is the memory size mt.DecrRef released larger?

@harshil-goel
Copy link
Contributor

@Lslightly I am not quite sure I understand what you mean. Could you elaborate?

@Lslightly
Copy link
Author

Lslightly commented Oct 22, 2024

@Lslightly I am not quite sure I understand what you mean. Could you elaborate?

The example of the reply in the Go forum can be more clear. For the db.imm case, the example can be changed to the following code. The example shows an extreme case when the size of the object(new(LargeT) in the following code) that mt points to is large and the object is kept alive by the underlying array of slice db.imm(s in the following code).

type LargeT struct { // assume `mt`'s type is `*LargeT` and the size of `LargeT` is large.
	arr [100000]int
}

func main() {
	s := f()
	var m runtime.MemStats
	for i := 0; i < 10; i++ {
		time.Sleep(100 * time.Millisecond)
		runtime.ReadMemStats(&m)
		fmt.Println(m.Alloc)
                // With setting nil, `new(LargeT)` can be reclaimed quickly.
                // Without setting nil, `new(LargeT)` has to be reclaimed when `s` is not used. 
		runtime.GC()
	}
	for i, elem := range s {
		elem.arr[0] = i
	}
}

func f() []*LargeT {
	len := 1000
	s := make([]*LargeT, len) // db.imm = make(...) here
	for i := 0; i < len; i++ {
		ptr := new(LargeT)
		ptr.arr[0] = i
		s[i] = ptr
	}

	start := len - 200 // `start` is 1 here

        // Here `db.imm[0]` is not set nil, which will keep the `new(LargeT)` alive
        // and not garbage collected. So I'm here asking whether the `db.imm` can
        // hold many references for a long time and whether the size of `mt` is large?
        // If not, i.e. `db.imm` holds few references or the size of `mt` is not so large,
        // there might be no "memory leak"-like problems.

	// comment the for loop for comparison
	for i := 0; i < start; i++ {
		s[i] = nil
	}
	s = s[start:] // For db.imm case, the code should be `db.imm = db.imm[1:]`. `start` is 1.
	return s
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants