Skip to content

ClearCache(), CacheInfo() and the LRU bound do not see the strongly-typed mapper cache #7

Description

@arnelirobles

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:

  1. 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.
  2. 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.
  3. 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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions