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

Clean WeakConcurrentMap from background thread #6240

Merged
merged 4 commits into from
Nov 23, 2022

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Jun 29, 2022

Currently our WeakConcurrentMap is only cleaned of stale entries when it is accessed. There is an option to clean from a background thread, but this creates a separate thread for every map. This pr introduces a single background thread that cleans all maps.
I removed the option to create a thread per map as we don't use it, if there is interest I could attempt to find a way to add it back.

@laurit laurit requested a review from a team June 29, 2022 15:18
@trask
Copy link
Member

trask commented Jun 29, 2022

Currently our WeakConcurrentMap is only cleaned of stale entries when it is accessed.

can you share thoughts / theories on why this is not enough? thx

@laurit
Copy link
Contributor Author

laurit commented Jun 30, 2022

Currently our WeakConcurrentMap is only cleaned of stale entries when it is accessed.

can you share thoughts / theories on why this is not enough? thx

This map keeps weak reference to keys and strong reference to values. Keys are collected automatically but values are cleared only when map is accessed. This does not guarantee that values are cleared in timely fashion or that they are cleared at all. Using a background thread allows for clearing values immediately after the key is collected.
This change is motivated by the recent question on slack about why we aren't using the cleaner threads. I am not aware of any issues that would require us to use a background thread, this just feels like a potential improvement to me.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

sorry about the long delay, it was not so scary after digging in a bit 👍

@mateuszrzeszutek mateuszrzeszutek linked an issue Nov 23, 2022 that may be closed by this pull request
@trask trask merged commit 52cfafc into open-telemetry:main Nov 23, 2022
@trask
Copy link
Member

trask commented Nov 23, 2022

thx @laurit ❤️

@lenin-jaganathan
Copy link

@mateuszrzeszutek / @trask We do Observe this particular issue causing a huge memory leak in some of our microservices. I am glad this is caught and fixed, but will this fix be backported to earlier versions? I am not aware of the version support process for OTEL agents. But just being curious about what happens to the customers on older versions.

@mateuszrzeszutek
Copy link
Member

Hey @lenin-jaganathan ,
No, we won't backport it. We only support the latest minor version; and we recommend using the latest minor version.

@laurit laurit deleted the weak-map-clean-thread branch March 14, 2023 13:40
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.

Reference Queue Excessive Memory Usage
4 participants