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

Possible Memory Leak with ConstantExpressionHelper #764

Closed
TWhidden opened this issue Jan 19, 2024 · 7 comments
Closed

Possible Memory Leak with ConstantExpressionHelper #764

TWhidden opened this issue Jan 19, 2024 · 7 comments
Labels

Comments

@TWhidden
Copy link
Contributor

Description
We are currently investigating a growing leak in our net8 app. Doing a dotnet-dump, and using JetBrains dotMemory, we found over 761k objects in the ConcurrentDictionary<object,Expression>, and 695k ConcurrentDictionary<Expression, string> both found in the ConstantExpressionHelper. We believe this is what is causing our leak. Analysis of all the instances show this class overwhelming on top of any other type instance.

Usage:
Our application makes heavy use of the dynamic library - where expression trees are created from input string data provided by outside calls. This has worked well for many years, going back to the early introduction of the dynamic linq library in the framework days.

Inputs change very regularly, based on the applications requirements. Constants are introduced that may only be used once in the application lifetime.

Possible Solution:
After cloning the project, was able to see the ConcurrentDictionary in ConstantExpressionHelper grow without any way to remove collected constant expressions. Proposal would be to use a sliding cache with a reasonable short lifetime (minutes? hours?), instead of an ever-lasting private static ConcurrentDictionary, allowing the system to remove constants that are no longer being called, which will then allow the GC to remove the tress of referenced objects.

Instance Details
Process running under Ubuntu Linux Container, net8.0 framework, approximately 9 days of uptime.

Trends monitoring:
image

Screen Shots of dotMemory outputs, arranged by size descending:
image

Event showing 18 megs of Int32 constants
image

The use of the ConstantExpressionHelper makes total sense, so I can appreciate why it's there. The proposal is just to have a way to let it clear itself out, especially for highly dynamic, long running services. Self-managing is desired, no reason for the developer to write code responsible to clear these things out.

I am more than willing to work on this and create a PR, downside is I don't have all the frameworks installed on my dev machine. Just getting it to compile required changing some csproj to remove all the old legacy things. If you would like a PR, let me know and ill work on that.

TWhidden pushed a commit to TWhidden/System.Linq.Dynamic.Core that referenced this issue Jan 19, 2024
@TWhidden
Copy link
Contributor Author

Ok, that was not so bad - I created a PR for review. Let me know if you want me to adjust or change anything.

#765

TWhidden pushed a commit to TWhidden/System.Linq.Dynamic.Core that referenced this issue Jan 19, 2024
TWhidden pushed a commit to TWhidden/System.Linq.Dynamic.Core that referenced this issue Jan 19, 2024
TWhidden pushed a commit to TWhidden/System.Linq.Dynamic.Core that referenced this issue Jan 19, 2024
TWhidden pushed a commit to TWhidden/System.Linq.Dynamic.Core that referenced this issue Jan 21, 2024
TWhidden pushed a commit to TWhidden/System.Linq.Dynamic.Core that referenced this issue Jan 21, 2024
TWhidden pushed a commit to TWhidden/System.Linq.Dynamic.Core that referenced this issue Jan 21, 2024
TWhidden pushed a commit to TWhidden/System.Linq.Dynamic.Core that referenced this issue Jan 22, 2024
TWhidden pushed a commit to TWhidden/System.Linq.Dynamic.Core that referenced this issue Jan 22, 2024
TWhidden pushed a commit to TWhidden/System.Linq.Dynamic.Core that referenced this issue Jan 22, 2024
TWhidden pushed a commit to TWhidden/System.Linq.Dynamic.Core that referenced this issue Jan 22, 2024
StefH pushed a commit that referenced this issue Jan 22, 2024
* #764 - Introduce Sliding Cache to Constant Expression Helper

* #764 Additional Tests, Missing TTL Update

* #764 - Rename T1,T2 to TKey, TValue

* #764 - Set cleanup time prior to enumeration to prevent multiple cleanup hits.

* #764 - Code Review Changes implemented; ThreadSafeSlidingCache Tests Added

* #764 - Move DefaultCleanupFrequency to internal static non generic class

* #764 - Dropped Preprocessor Directive for TTL in ConstantExpressionHelper

* #764 - Move ConstantExpressionHelper to Instance. Refactor feedback from Review

* #764 - ConstantExpressionHelper Singleton Factory; Code Review Resolutions

* #764 - Refactor Naming for SlidingCache;

* #764 - PR Refactor Feedback

---------

Co-authored-by: Travis Whidden <[email protected]>
@StefH StefH added the bug label Jan 22, 2024
@StefH
Copy link
Collaborator

StefH commented Jan 23, 2024

@TWhidden
Preview version can be tested: 1.3.9-preview-01

@StefH
Copy link
Collaborator

StefH commented Jan 31, 2024

@TWhidden
Did you have time to test this?

@TWhidden
Copy link
Contributor Author

Got into prod last week.I'll dump the process today and do an analysis.

@TWhidden
Copy link
Contributor Author

TWhidden commented Feb 1, 2024

I'd say we made a huge difference. This is 7 days of uptime on a busy server. Do I close this issue or do you?
image

@StefH
Copy link
Collaborator

StefH commented Feb 1, 2024

You can close it. Or I will.

Thanks for everything.

@TWhidden
Copy link
Contributor Author

TWhidden commented Feb 2, 2024

We detected a runtime problem with our Sentry logging. Since this PR was already merged, I created a new issue and a new PR #769 - I hope that is the right way to handle it. Glad it was just a pre-release!

@TWhidden TWhidden closed this as completed Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants