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

Fix memory leak in queue #4192

Merged
merged 1 commit into from
Jan 24, 2025
Merged

Fix memory leak in queue #4192

merged 1 commit into from
Jan 24, 2025

Conversation

ankur22
Copy link
Contributor

@ankur22 ankur22 commented Jan 17, 2025

What?

Overwrite the empty queue.read with a new slice.

Why?

When we read off the queue.read slice, it essentially pops the first available element by doing this:

case eh.ch <- eh.queue.read[0]:
        eh.queue.read[0] = Event{}
	eh.queue.read = eh.queue.read[1:]

(link to code)

So the entry where the event used to exist has been overwritten with a fresh instance of Event which will help avoid a memory leak (which was recently fixed). However the issue is that the underlying slice keeps growing. During a long running test the slice could grow to an unnecessarily length.

To avoid this issue, when queue.read is empty and is swapped with queue.write, just before swapping them around the existing queue.read is overwritten with a new slice, freeing up the old potentially long slice. This should help avoid further memory leaks.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

@ankur22 ankur22 requested a review from a team as a code owner January 17, 2025 10:54
@ankur22 ankur22 requested review from oleiade, codebien and inancgumus and removed request for a team January 17, 2025 10:54
@ankur22 ankur22 marked this pull request as draft January 17, 2025 10:55
@ankur22 ankur22 changed the base branch from master to remove/events January 17, 2025 10:55
@ankur22 ankur22 marked this pull request as ready for review January 17, 2025 11:00
@ankur22 ankur22 changed the title Fix mem leak Fix memory leak in queue Jan 17, 2025
inancgumus
inancgumus previously approved these changes Jan 17, 2025
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

Interesting! 👍 Btw, the older commits from that other PR are still showing up in this PR.

@joanlopez
Copy link
Contributor

However the issue is that the underlying slice keeps growing.

Does that mean that the Go GC isn't able to collect the memory allocations that are no longer pointed by any slice? 🤔

I guess that after:

eh.queue.read = eh.queue.read[1:]

The memory address where eh.queue.read[0] is stored remains, but since after that line there's no slice pointing to that position (eh.queue.read now points to, since the next position eh.queue.read[1] till the end of the underlying array), I'd expect the GC to be smart enough to collect these addresses.

Instead, I guess that (based on the change on this PR), it is only able to collect a range of memory addresses that's not pointed at all by any slice. Is that reasoning right? 🤔

joanlopez
joanlopez previously approved these changes Jan 20, 2025
@ankur22
Copy link
Contributor Author

ankur22 commented Jan 20, 2025

@joanlopez the GC will only cleanup the element that the 0th entry is pointing to if nothing is referencing it. So the underlying array remains in place i believe.

In grafana/xk6-browser#1488, we found this the hard way when the reference was being kept even though there was no way to get back to the 0th element after performing

eh.queue.read = eh.queue.read[1:]

Therefore by making the change in this PR, we are sure that the underlying array is empty and the previous array is cleaned up by the GC.

@ankur22 ankur22 force-pushed the remove/events branch 2 times, most recently from d8a6212 to 638eb16 Compare January 21, 2025 10:22
Base automatically changed from remove/events to master January 22, 2025 09:33
@ankur22 ankur22 dismissed stale reviews from inancgumus and joanlopez January 22, 2025 09:33

The base branch was changed.

joanlopez
joanlopez previously approved these changes Jan 22, 2025
When we read off the queue.read slice, it essentially pops the first
available element by doing this:

case eh.ch <- eh.queue.read[0]:
        eh.queue.read[0] = Event{}
	eh.queue.read = eh.queue.read[1:]

So the entry where the event used to exist has been overwritten with a
fresh instance of Event which will avoid a memory leak. However the
issue is that the underlying slice keeps growing. During a long running
test the slice could grow to an unnecessarily length.

To avoid this issue, when queue.read is empty and is swapped with
queue.write, just before swapping them around the existing queue.read
is overwritten with a new slice, freeing up the old potentially long
slice. This should help avoid memory leaks.
@ankur22 ankur22 merged commit f512893 into master Jan 24, 2025
29 checks passed
@ankur22 ankur22 deleted the fix/mem-leak branch January 24, 2025 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants