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.

Sign up for GitHub

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

Jump to bottom

Fixed memory leak due to infinite static caches #2438

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fixed memory leak due to infinite static caches #2438

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1061987
Fixed memory leak due to infinite static caches
Feb 26, 2024
8e7dd71
Parametrize sliding expiration interval
Mar 28, 2024
96b6612
Revert "Fixed memory leak due to infinite static caches"
alexandr-h May 27, 2024
1b6d98e
Implement basic SlidingCache
alexandr-h May 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 11 additions & 6 deletions LiteDB/Document/Expression/BsonExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Reflection;
using System.Text;
using System.Text.RegularExpressions;
using Microsoft.Extensions.Caching.Memory;
using static LiteDB.Constants;

namespace LiteDB
Expand Down Expand Up @@ -279,8 +280,8 @@ internal BsonValue ExecuteScalar(IEnumerable<BsonDocument> source, BsonDocument

#region Static method

private static readonly ConcurrentDictionary<string, BsonExpressionEnumerableDelegate> _cacheEnumerable = new ConcurrentDictionary<string, BsonExpressionEnumerableDelegate>();
private static readonly ConcurrentDictionary<string, BsonExpressionScalarDelegate> _cacheScalar = new ConcurrentDictionary<string, BsonExpressionScalarDelegate>();
private static readonly IMemoryCache _cacheEnumerable = new MemoryCache(new MemoryCacheOptions());
private static readonly IMemoryCache _cacheScalar = new MemoryCache(new MemoryCacheOptions());

/// <summary>
/// Parse string and create new instance of BsonExpression - can be cached
Expand Down Expand Up @@ -358,9 +359,11 @@ internal static void Compile(BsonExpression expr, ExpressionContext context)
// in both case, try use cached compiled version
if (expr.IsScalar)
{
var cached = _cacheScalar.GetOrAdd(expr.Source, s =>
var cached = _cacheScalar.GetOrCreate(expr.Source, cacheEntry =>
{
var lambda = System.Linq.Expressions.Expression.Lambda<BsonExpressionScalarDelegate>(expr.Expression, context.Source, context.Root, context.Current, context.Collation, context.Parameters);
cacheEntry.SlidingExpiration = TimeSpan.FromMinutes(1);

var lambda = Expression.Lambda<BsonExpressionScalarDelegate>(expr.Expression, context.Source, context.Root, context.Current, context.Collation, context.Parameters);

return lambda.Compile();
});
Expand All @@ -369,9 +372,11 @@ internal static void Compile(BsonExpression expr, ExpressionContext context)
}
else
{
var cached = _cacheEnumerable.GetOrAdd(expr.Source, s =>
var cached = _cacheEnumerable.GetOrCreate(expr.Source, cacheEntry =>
{
var lambda = System.Linq.Expressions.Expression.Lambda<BsonExpressionEnumerableDelegate>(expr.Expression, context.Source, context.Root, context.Current, context.Collation, context.Parameters);
cacheEntry.SlidingExpiration = TimeSpan.FromMinutes(1);

var lambda = Expression.Lambda<BsonExpressionEnumerableDelegate>(expr.Expression, context.Source, context.Root, context.Current, context.Collation, context.Parameters);

return lambda.Compile();
});
Expand Down
13 changes: 11 additions & 2 deletions LiteDB/LiteDB.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFrameworks>net4.5;netstandard1.3;netstandard2.0</TargetFrameworks>
<TargetFrameworks>net4.5.1;netstandard1.3;netstandard2.0</TargetFrameworks>
<AssemblyVersion>5.0.19</AssemblyVersion>
<FileVersion>5.0.19</FileVersion>
<VersionPrefix>5.0.19</VersionPrefix>
Expand Down Expand Up @@ -56,15 +56,24 @@
<None Include="..\icon_64x64.png" Pack="true" PackagePath="\" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'net45'">
<ItemGroup Condition="'$(TargetFramework)' == 'net4.5.1'">
<Reference Include="System" />
<Reference Include="System.Runtime" />
<PackageReference Include="Microsoft.Extensions.Caching.Memory" Version="1.1.2" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard1.3'">
<PackageReference Include="System.Reflection.TypeExtensions" Version="4.5.1" />
<PackageReference Include="System.Security.Cryptography.Algorithms" Version="4.3.1" />
<PackageReference Include="Microsoft.Extensions.Caching.Memory" Version="1.1.2" />
Copy link

Choose a reason for hiding this comment

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

I am not a maintainer of this lib, but I doubt adding Microsoft.Extensions.Caching.Memory dependency would be approved. Implementing cache expiration logic relying on what is available in BCL may be needed.
Also, since this is a general purpose library, few concerns regarding sliding cache expiration approach:

  1. It introduces implicit dependency on static DateTime.Now or similar, which may become a problem for unit testing (not only LiteDB itself, but any other projects that uses LiteDB in tests). Microsoft.Extensions.Caching.Memory has an ISystemClock abstraction. If LiteDB library would rely on system time for caching logic, there should be a way to supply similar abstraction to LiteDB, so implementation could be substituted if needed.
  2. The hard decision to have sliding expiration exactly to 1 minute may need to be parametrized.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your review.
"dependency would be approved" - Absolutely agree. I'll try to rewrite this when I have some free time.
"which may become a problem for unit testing" - I absolutely agree, but I still have no idea how to fix this without seriously reworking the project architecture. Static methods and static caches are not the best solution.
"need to be parametrized" - Thanks, done. At the same time, now by default the parameter does not change the existing logic - indefinite cache

</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<PackageReference Include="System.Reflection.TypeExtensions" Version="4.5.1" />
<PackageReference Include="System.Security.Cryptography.Algorithms" Version="4.3.1" />
<PackageReference Include="Microsoft.Extensions.Caching.Memory" Version="8.0.0" />
</ItemGroup>


<!-- End References -->

Expand Down