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 enqueue/dequeue of EventPipe callback data. #58170

Merged

Conversation

lateralusX
Copy link
Member

#56104 made sure provider callback data gets its own copy of filter data. This created a couple of memory leaks when queue/dequeue the callback data since callback data was not correctly freed in these scenarios leading to leaks of the copied filter data.

Was detected running the manual EventPipe native unit tests on Windows using its build in use of _CrtMemCheckpoint (only available in debug builds) automatically detecting memory leaks.

Fix makes sure callback data is moved into queue on enqueue and moved out in dequeue and that caller of dequeue make sure to free returned callback data using ep_provider_callback_data_fini when done using it. Doing a move instead of copy will also reduce the number of allocations when enqueue/dequeue callback data in provider callback queue.

dotnet#56104 made sure provider
callback data gets its own copy of filter data. This created a couple
of memory leaks when queue/dequeue the callback data since callback data
was not correctly freed in these scenarios leading to leaks of the copied
filter data.

Was detected running the manual EventPipe native unit tests on Windows
using its build in use of _CrtMemCheckpoint (only available in debug
builds) automatically detecting memory leaks.

Fix makes sure callback data is moved into queue on enqueue and moved
out in dequeue and that caller of dequeue make sure to
free returned callback data using ep_provider_callback_data_fini
when done using it. Doing a move instead of copy will also reduce
the number of allocations when enqueue/dequeue callback data in
provider callback queue.
@lateralusX
Copy link
Member Author

//CC @josalem

Copy link
Contributor

@josalem josalem left a comment

Choose a reason for hiding this comment

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

Thanks @lateralusX!

@lateralusX
Copy link
Member Author

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1173315940

@lateralusX lateralusX merged commit 3cd4e39 into dotnet:main Aug 27, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants