-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Keep actual parameter values out of CommandCacheKey in RelationalCommandCache #34631
Conversation
Only store necessary info for paramter values RelationalCommandCacheKey to prevent memory leaks.
@dotnet-policy-service agree |
@@ -106,11 +106,16 @@ void IPrintableExpression.Print(ExpressionPrinter expressionPrinter) | |||
} | |||
} | |||
|
|||
private readonly struct CommandCacheKey(Expression queryExpression, IReadOnlyDictionary<string, object?> parameterValues) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameterValues contains all user data objects in queries before AsEnumerable or ToList, handle as radioactive. We only need meta data anyway
It may not seem much, but this a confirmed problem in production for us. Clearing IMemoryCache instantly drops memory by 600mb in some cases. If you are unlucky and a huge query is executed while creating this cache entry for the first time, all those entities will remain in memory through some wierd links that GC deems still alive objects. To debug the issue we used optionsBuilder.UseMemoryCache(_memoryCache); to easily be able to inspect the same IMemoryCache as EF uses |
// ReSharper disable once ArrangeRedundantParentheses | ||
if ((value == null) != (otherValue == null)) | ||
{ | ||
if (value != otherValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep old behaviour, where we return true on first object[], or compare all values?
This PR lacks unit tests to verify that it fixes the memory leak issue described. |
@cliffankh0z you can just abandon this PR now in favor of: Thank you very much for doing the initial work for this but I've got it now reproduced with unit tests and fixed as well. |
Memory leak when storing the actual parameter values in MemoryCache in CommandCacheKey. Parameter values can be anything used as parameteres for the queries; lists of Guids, even objects used for entity inits etc. This keeps them from being garbage collected and thereby creating memory leaks. Store only what we actually need to compare parameter values.
Anyway, the intent of the code is clear. If a maintainer could fix this in the correct manner and style that would be greatly appreciated!
#34028