MapTo<TSource, TDest>() stores its compiled delegate in a static generic class that none of the
cache management APIs know about. Three observable consequences: CacheInfo() under-reports,
ClearCache() does not clear it, and UseLruCache/MaxCacheSize do not bound it.
Reproduction:
Mapper.ClearCache();
var dto = new Source { A = 1 }.MapTo<Source, Dest>();
Console.WriteLine(Mapper.CacheInfo().Total); // 0, although a compiled mapper is now cached
Mapper.UseLruCache = true;
Mapper.MaxCacheSize = 1;
_ = new Source { A = 2 }.MapTo<Source, Dest>();
Console.WriteLine(Mapper.CacheInfo().Total); // still 0; the LRU bound never applies to this path
Current behaviour. src/Mapsicle/Mapsicle.cs:426:
private static class TypedMapperCache<TSource, TDest>
{
private static volatile TypedMapperCacheEntry<TSource, TDest>? _entry;
public static TypedMapperCacheEntry<TSource, TDest>? Entry => _entry;
public static void Initialize(Func<TSource, TDest> mapper, bool requiresDepthTracking)
{
_entry = new TypedMapperCacheEntry<TSource, TDest>(mapper, requiresDepthTracking);
}
}
Per-closed-generic static state is the reason this path is fast: the lookup is a static field read
with no dictionary and no tuple key, and the measured warm cost is 32 to 48 bytes per call, all of it
the destination object. That is worth preserving.
But ClearCache() at src/Mapsicle/Mapsicle.cs:195 clears _mapToCache, _mapCache, the three
PropertyInfo caches and _needsDepthTrackingCache, and has no way to reach
TypedMapperCache<,>. CacheInfo() at line 215 reports only the dictionary counts.
ReinitializeCaches() at line 165 has the same blind spot, so toggling UseLruCache does not affect
the typed path at all.
In an application that maps a bounded set of type pairs this is harmless. In one that closes generics
over many pairs, the typed cache is a permanent, unbounded, unreportable retention of compiled
delegates, which is precisely what MaxCacheSize exists to prevent.
What should change. This needs a decision before code, so please comment before opening a PR.
Three options, in increasing cost:
- Register on initialize. Keep the static generic field for the fast read, and additionally
record a clear action in a static list at Initialize time so ClearCache() can walk it and
CacheInfo() can count it. Costs one registration per type pair, once. Fast path unchanged.
- Version stamp. Add a static
_cacheGeneration counter that ClearCache() increments; the
entry records the generation it was built in and the fast path compares. Costs one integer
comparison per call. Bounding still does not work.
- Document it as intentional and rename it in the docs: the typed path is a permanent
per-type-pair cache and is not subject to MaxCacheSize. Cheapest, and honest, but it makes the
README's "Memory Bounded" row conditional, which needs saying there too.
Option 1 is my suggestion; it preserves the measured allocation numbers and makes both APIs true.
How you know you succeeded. In tests/Mapsicle.Tests/ProductionScenarioTests.cs, which already
has cache assertions and carries [Collection("StaticMapperTests")]:
[Fact]
public void CacheInfo_CountsTypedMappers()
{
Mapper.ClearCache();
_ = new TypedCacheSource { A = 1 }.MapTo<TypedCacheSource, TypedCacheDest>();
Assert.True(Mapper.CacheInfo().Total > 0);
}
[Fact]
public void ClearCache_ClearsTypedMappers()
{
_ = new TypedCacheSource { A = 1 }.MapTo<TypedCacheSource, TypedCacheDest>();
Mapper.ClearCache();
Assert.Equal(0, Mapper.CacheInfo().Total);
}
The first fails today (Total is 0); the second passes today for the wrong reason, which is why
they belong together.
Whatever option is chosen, add an assertion to tests/Mapsicle.Performance.Tests (see issue 10) that
the typed warm path still allocates no more than 64 bytes per call. The point of the fix is to make
the bookkeeping honest, not to move the fast path.
Non-obvious thing. ProductionScenarioTests.cs:327 currently asserts
Assert.Equal(0, infoAfter.Total) after a ClearCache(). If option 1 lands, check whether any test
in that file mapped through the typed path first; that assertion is only true today because the typed
path is invisible.
MapTo<TSource, TDest>()stores its compiled delegate in a static generic class that none of thecache management APIs know about. Three observable consequences:
CacheInfo()under-reports,ClearCache()does not clear it, andUseLruCache/MaxCacheSizedo not bound it.Reproduction:
Current behaviour.
src/Mapsicle/Mapsicle.cs:426:Per-closed-generic static state is the reason this path is fast: the lookup is a static field read
with no dictionary and no tuple key, and the measured warm cost is 32 to 48 bytes per call, all of it
the destination object. That is worth preserving.
But
ClearCache()atsrc/Mapsicle/Mapsicle.cs:195clears_mapToCache,_mapCache, the threePropertyInfocaches and_needsDepthTrackingCache, and has no way to reachTypedMapperCache<,>.CacheInfo()at line 215 reports only the dictionary counts.ReinitializeCaches()at line 165 has the same blind spot, so togglingUseLruCachedoes not affectthe typed path at all.
In an application that maps a bounded set of type pairs this is harmless. In one that closes generics
over many pairs, the typed cache is a permanent, unbounded, unreportable retention of compiled
delegates, which is precisely what
MaxCacheSizeexists to prevent.What should change. This needs a decision before code, so please comment before opening a PR.
Three options, in increasing cost:
record a clear action in a static list at
Initializetime soClearCache()can walk it andCacheInfo()can count it. Costs one registration per type pair, once. Fast path unchanged._cacheGenerationcounter thatClearCache()increments; theentry records the generation it was built in and the fast path compares. Costs one integer
comparison per call. Bounding still does not work.
per-type-pair cache and is not subject to
MaxCacheSize. Cheapest, and honest, but it makes theREADME's "Memory Bounded" row conditional, which needs saying there too.
Option 1 is my suggestion; it preserves the measured allocation numbers and makes both APIs true.
How you know you succeeded. In
tests/Mapsicle.Tests/ProductionScenarioTests.cs, which alreadyhas cache assertions and carries
[Collection("StaticMapperTests")]:The first fails today (
Totalis0); the second passes today for the wrong reason, which is whythey belong together.
Whatever option is chosen, add an assertion to
tests/Mapsicle.Performance.Tests(see issue 10) thatthe typed warm path still allocates no more than 64 bytes per call. The point of the fix is to make
the bookkeeping honest, not to move the fast path.
Non-obvious thing.
ProductionScenarioTests.cs:327currently assertsAssert.Equal(0, infoAfter.Total)after aClearCache(). If option 1 lands, check whether any testin that file mapped through the typed path first; that assertion is only true today because the typed
path is invisible.