fix: memory leaks. #2489

docmerlin · 2021-02-26T16:36:51Z

There were a number of memory leaks of the form of a = a[i:].
These were replaced with grow-able circular queue.

lesam · 2021-02-26T17:36:43Z

Could we extract the pattern? i.e.

a.PushBack(<slice>)
a.PopFront(<slice>)
a.Peek(index) <slice>

We already have codegen tools for the various slice types and it would make me feel better.

nathanielc

LGTM, in short these buffers grow indefinitely so when we reslice them to only the end of the buffer we need to shift all the data to the front of the buffer otherwise Go cannot free the unused space.

These appends therefore do not do any new allocations rather move the data to the beginning of the slice

lesam · 2021-02-26T17:37:04Z

Also this repo should be review-protected!

lesam

see comment

lesam · 2021-02-26T17:42:03Z

Mostly I'm worried about someone relying on these slices not being mutated, if we had the pattern extracted that would prevent future errors. i.e. these two are not equivalent:

a := []int{1,2,3}
b := a[1:]
fmt.Println(a[0])

a := []int{1,2,3}
b := append(a[0:], a[:1]...)
fmt.Println(a[0])

lesam · 2021-02-26T17:47:10Z

Possibly a better interface:

a.PushBack(<slice>)
a.PopFront()
a.PeekFront() <value>

docmerlin · 2021-03-02T22:30:10Z

circularqueue.gen_test.go

+			endL := len(v) + q.l - q.tail
+			q.data = append(q.data[:0], append(v[endL:], append(q.data[q.head:q.tail], v[:endL]...)...)...)
+			q.tail += endL
+			break


This may be a bit overengineered, I ended up not needing more than 1 enqueue at a time. I am not sure if I should remove the ability to enqueue multiple items at once.

I vote for single enqueue - feel free to save the multi-enqueue version somewhere in case we need it in the future. Much easier to test single enqueue.

Easier for me to review as well 😄

Backed up to: https://gist.github.com/docmerlin/b29b9a36c120e239b1e6c348d09dc0c1

docmerlin · 2021-03-02T22:31:01Z

circularqueue_test.go

+		i := i // make i a local object
+		item := circularIntPtr(&i)
+		runtime.SetFinalizer(item, func(q circularIntPtr) {
+			// if the finalizer is called, that means that the GC believes the objects should be freed


We use runtime.SetFinalizer to test if the GC is seeing the objects for deletion.

docmerlin · 2021-03-02T22:55:37Z

join.go

-			// Can't trust lowMark until all parents have reported.
-			// Remove any unneeded match points.
-			n.matchGroupsBuffer[groupId] = matches[i:]
-		}


This block above, looks like it creates duplicates when stuff hasn't reported. I removed it, it could be related to an issue that has been seen.

circularqueue.gen_test.go

lesam · 2021-03-03T14:17:55Z

join_circularqueues.gen.go

+
+// newSrcPointconstructs a Circular Queue
+// with a given buffer buf.  It is ok for buf to be nil.
+func newSrcPointCircularQueue(buf ...srcPoint) *srcPointCircularQueue {


Looks like this is never called?

lesam · 2021-03-17T20:17:13Z

circularqueue.gen.go.tmpl

+
+// Peek peeks i ahead of the current head of queue.  It should be used in conjunction with .Len() to prevent a panic.
+func (q *{{ $k }}CircularQueue) Peek(i int) {{ $k }} {
+	p := q.head + i


Should we bounds check to make sure we don't get garbage?

It's intended to be used in conjuction with .Len to avoid that.

Ok added the bounds check.

lesam · 2021-03-17T20:24:00Z

circularqueue.gen.go.tmpl

+		if q.head < q.tail{
+			copy(buf, q.data[q.head:q.tail])
+		} else {
+			copy(buf[copy(buf, q.data[q.head:]):], q.data[:q.tail])


This might be easier to understand written on more than one line:

partialLength := copy(buf, q.data[q.head:]) copy(buf[partialLength):], q.data[:q.tail])

lesam · 2021-03-17T20:26:16Z

circularqueue_test.go

+
+	}
+
+	//if q.Dequeue(1); q.Peek(-1) != q.Val() {


commented code ?

lesam · 2021-03-17T20:29:10Z

circularqueue_test.go

+	}
+}
+
+func Test_leakCircularBuf(t *testing.T) {


It might be easier to use Benchmarks for this sort of test, it gives you the number of allocations. (for the future, this is fine as-is)

This isn't for testing allocations, but for testing to make sure things get properly de-allocated (a test against leaks).

lesam · 2021-03-17T20:34:42Z

circularqueue.gen.go.tmpl

+type {{ $k }}CircularQueue struct {
+	data     []{{ $k }}
+	head     int
+	tail     int


invariant: tail = (head + l) % len(data) right? Is it worth keeping it separately?

Yes, its mostly so we don't have to do a mod over and over again, it isn't strictly necessary.

lesam · 2021-03-17T20:37:23Z

circularqueue.gen.go.tmpl

+}
+
+// Peek peeks i ahead of the current head of queue.  It should be used in conjunction with .Len() to prevent a panic.
+func (q *{{ $k }}CircularQueue) Peek(i int) {{ $k }} {


This might be better named At since we're only showing one value at an index, Peek sounds like it should return a slice. Not a blocker.

I think I will keep it this same.

There were a number of memory leaks of the form of `a = a[i:]`. These were replaced with circular queue buffers, to prevent the leaks.

nathanielc approved these changes Feb 26, 2021

View reviewed changes

lesam suggested changes Feb 26, 2021

View reviewed changes

docmerlin force-pushed the fix-memory-leak branch from 66a8c1e to 42667a2 Compare March 2, 2021 21:19

docmerlin commented Mar 2, 2021

View reviewed changes

docmerlin force-pushed the fix-memory-leak branch from 09a3ed3 to b4c3bbb Compare March 3, 2021 00:51

lesam reviewed Mar 3, 2021

View reviewed changes

circularqueue.gen_test.go Show resolved Hide resolved

lesam reviewed Mar 3, 2021

View reviewed changes

docmerlin mentioned this pull request Mar 3, 2021

unbounded memory growth to OOM with relatively simple scripts on version 1.5.7 #2471

Open

docmerlin force-pushed the fix-memory-leak branch 3 times, most recently from c6d819c to d0bd4a5 Compare March 4, 2021 23:45

docmerlin force-pushed the fix-memory-leak branch 5 times, most recently from 6465890 to c43b954 Compare March 17, 2021 19:31

lesam reviewed Mar 17, 2021

View reviewed changes

docmerlin force-pushed the fix-memory-leak branch from c43b954 to edff10c Compare March 17, 2021 20:33

lesam reviewed Mar 17, 2021

View reviewed changes

fix: memory leaks

85d0821

There were a number of memory leaks of the form of `a = a[i:]`. These were replaced with circular queue buffers, to prevent the leaks.

docmerlin force-pushed the fix-memory-leak branch from edff10c to 85d0821 Compare March 17, 2021 20:44

lesam approved these changes Mar 17, 2021

View reviewed changes

docmerlin merged commit d06d07c into master Mar 17, 2021

docmerlin deleted the fix-memory-leak branch March 17, 2021 21:12

alespour pushed a commit to bonitoo-io/kapacitor that referenced this pull request Mar 24, 2021

fix: memory leaks (influxdata#2489)

189d300

There were a number of memory leaks of the form of `a = a[i:]`. These were replaced with circular queue buffers, to prevent the leaks.

joshdurbin mentioned this pull request Mar 29, 2021

JoinNode ignores Delete BarrierNode messages. #2436

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: memory leaks. #2489

fix: memory leaks. #2489

docmerlin commented Feb 26, 2021 •

edited

Loading

lesam commented Feb 26, 2021

nathanielc left a comment

lesam commented Feb 26, 2021

lesam left a comment

lesam commented Feb 26, 2021 •

edited

Loading

lesam commented Feb 26, 2021 •

edited

Loading

docmerlin Mar 2, 2021

lesam Mar 3, 2021

lesam Mar 3, 2021

docmerlin Mar 3, 2021

docmerlin Mar 2, 2021

docmerlin Mar 2, 2021

lesam Mar 3, 2021

lesam Mar 17, 2021

docmerlin Mar 17, 2021

docmerlin Mar 17, 2021

lesam Mar 17, 2021

lesam Mar 17, 2021

lesam Mar 17, 2021

docmerlin Mar 17, 2021

lesam Mar 17, 2021

docmerlin Mar 17, 2021 •

edited

Loading

lesam Mar 17, 2021

docmerlin Mar 17, 2021

fix: memory leaks. #2489

fix: memory leaks. #2489

Conversation

docmerlin commented Feb 26, 2021 • edited Loading

lesam commented Feb 26, 2021

nathanielc left a comment

Choose a reason for hiding this comment

lesam commented Feb 26, 2021

lesam left a comment

Choose a reason for hiding this comment

lesam commented Feb 26, 2021 • edited Loading

lesam commented Feb 26, 2021 • 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

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

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

docmerlin Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

docmerlin commented Feb 26, 2021 •

edited

Loading

lesam commented Feb 26, 2021 •

edited

Loading

lesam commented Feb 26, 2021 •

edited

Loading

docmerlin Mar 17, 2021 •

edited

Loading