move to TUnit, fix thread safety edge cases#34
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the test suite to TUnit / Microsoft.Testing.Platform and refactors FastCloner’s runtime configuration + caches to address thread-safety/configuration edge cases by using immutable config snapshots and config-keyed caches.
Changes:
- Introduce
FastClonerRuntimeConfigsnapshots,FastClonerPublishedEngine, and a thread-staticFastClonerRuntimeConfigScope. - Version cache entries (and some “safe type” decisions) by a configuration
CacheKeyto avoid cross-config contamination. - Migrate tests from NUnit-style assertions/fixtures to TUnit-style async assertions and lifecycle attributes; update test runner configuration.
Reviewed changes
Copilot reviewed 47 out of 48 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/global.json | Configures the test runner (Microsoft.Testing.Platform). |
| src/FastCloner/FastCloner.cs | Publishes immutable runtime config snapshots; updates public config/type-behavior APIs. |
| src/FastCloner/Code/FastClonerRuntimeConfig.cs | Adds runtime config snapshot + published engine + thread-static scope types. |
| src/FastCloner/Code/FastClonerCache.cs | Introduces config-keyed (“versioned”) caches and config-aware behavior queries. |
| src/FastCloner/Code/FastClonerSafeTypes.cs | Splits known-types cache into default vs config-keyed results. |
| src/FastCloner/Code/FastClonerExprGenerator.cs | Keys adaptive dictionary processor factory cache by config cache key. |
| src/FastCloner/Code/FastCloneState.cs | Threads config through cloning state (thread-static pools/simple state). |
| src/FastCloner.Tests/FastCloner.Tests.csproj | Replaces NUnit packages with TUnit; targets net10.0. |
| src/FastCloner.Tests/GlobalUsings.cs | Removes NUnit global using (tests rely on new framework setup). |
| src/FastCloner.Tests/BaseTestFixture.cs | Replaces NUnit fixtures with TUnit [Arguments] and setup/teardown attributes. |
| src/FastCloner.Tests/TypeTests.cs | Migrates assertions/metadata to TUnit async asserts/properties. |
| src/FastCloner.Tests/SourceGeneratorTests.cs | Migrates to async TUnit asserts; minor test body adjustments. |
| src/FastCloner.Tests/SourceGeneratorSafetyTests.cs | Migrates assertions and Task member usage to TUnit style. |
| src/FastCloner.Tests/SourceGeneratorGenericTests.cs | Migrates to async TUnit asserts; updates collection literals. |
| src/FastCloner.Tests/SourceGeneratorCrossNamespaceTests.cs | Migrates to async TUnit asserts; simplifies regions/collections. |
| src/FastCloner.Tests/SourceGeneratorCompatibleAttribute.cs | Adjusts attribute usage metadata for new test framework conventions. |
| src/FastCloner.Tests/ShallowCloneTests.cs | Migrates to async TUnit asserts; updates numeric literal typing. |
| src/FastCloner.Tests/RecordTests.cs | Migrates to async TUnit asserts. |
| src/FastCloner.Tests/ObjectTests.cs | Migrates to async TUnit asserts; retains existing test coverage. |
| src/FastCloner.Tests/InheritanceTests.cs | Migrates to async TUnit asserts. |
| src/FastCloner.Tests/IncludeAttributeTests.cs | Migrates to async TUnit asserts. |
| src/FastCloner.Tests/ImplicitCollectionTests.cs | Migrates to async TUnit asserts; updates collection literals. |
| src/FastCloner.Tests/IdentityPreservationTests.cs | Migrates to async TUnit asserts; updates collection initializers. |
| src/FastCloner.Tests/GenericTests.cs | Migrates to async TUnit asserts; updates reference-equality assertions. |
| src/FastCloner.Tests/FailureHypothesisTests.cs | Migrates to async TUnit asserts; updates collection literals/usings. |
| src/FastCloner.Tests/DynamicMaxRecursionDepthTests.cs | Updates lifecycle hooks and assertions for TUnit; resets global state after each test. |
| src/FastCloner.Tests/DiagnosticTests.cs | Migrates diagnostic assertions to async TUnit style; modernizes collection literals. |
| src/FastCloner.Tests/DbTests.cs | Converts to TUnit lifecycle; makes session factory static; adds non-parallel marker. |
| src/FastCloner.Tests/CtorTests.cs | Migrates concurrency tests/assertions to async TUnit style. |
| src/FastCloner.Tests/ContextTests.cs | Migrates to async TUnit asserts for context cloning behaviors. |
| src/FastCloner.Tests/ConcurrentTests.cs | Migrates to async TUnit asserts; adjusts concurrency coordination. |
| src/FastCloner.Tests/CollectionTests.cs | Migrates to async TUnit asserts for collection cloning tests. |
| src/FastCloner.Tests/CircularTests.cs | Migrates to async TUnit asserts for cycle handling tests. |
| src/FastCloner.Tests/BenchmarkModelTests.cs | Migrates to async TUnit asserts for benchmark model validation. |
| src/FastCloner.Tests/BehaviorAttributeTests.cs | Migrates to async TUnit asserts for attribute-driven behavior tests. |
| src/FastCloner.Tests/ArrayTests.cs | Migrates to async TUnit asserts; updates collection assertions and literals. |
| src/FastCloner.Tests/AdvancedCollectionTests.cs | Migrates to async TUnit asserts; updates collection literals. |
| src/FastCloner.Tests/AbstractClassTests.cs | Migrates to async TUnit asserts across polymorphic/abstract scenarios. |
| src/FastCloner.Benchmark/Program.cs | Minor cleanup + pattern matching tweak; makes Program internal. |
| .bench-harness-current/Program.cs | Adds a simple benchmark harness for quick local perf checks. |
| .bench-harness-current/BenchHarness.csproj | Adds bench harness project referencing FastCloner. |
Comments suppressed due to low confidence (1)
src/FastCloner.Tests/DbTests.cs:116
sessionFactoryis nullable (ISessionFactory?), but it's dereferenced without a null-forgiving operator or guard (e.g.,sessionFactory.OpenSession()). With nullable enabled this will generate warnings (and potentially fail builds if warnings are treated as errors). IfSetUpguarantees initialization, consider making it non-nullable and assigning inSetUp(or usesessionFactory!at dereference sites).
{
using ISession? session = sessionFactory.OpenSession();
using ITransaction? transaction = session.BeginTransaction();
try
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private readonly ConcurrentDictionary<IntPtr, TValue> defaultCache = new ConcurrentDictionary<IntPtr, TValue>(); | ||
| private readonly ConcurrentDictionary<ConfigTypeKey, TValue> versionedCache = new ConcurrentDictionary<ConfigTypeKey, TValue>(); | ||
|
|
||
| public TValue GetOrAdd(Type type, Func<Type, TValue> valueFactory) | ||
| { | ||
| IntPtr handle = type.TypeHandle.Value; | ||
| return cache.TryGetValue(handle, out TValue? existing) ? existing : cache.GetOrAdd(handle, _ => valueFactory(type)); | ||
| long cacheKey = GetAmbientCacheKey(); | ||
| if (cacheKey == 0) | ||
| { | ||
| return defaultCache.TryGetValue(handle, out TValue? existing) | ||
| ? existing | ||
| : defaultCache.GetOrAdd(handle, _ => valueFactory(type)); | ||
| } | ||
|
|
||
| return versionedCache.GetOrAdd(new ConfigTypeKey(cacheKey, handle), _ => valueFactory(type)); | ||
| } | ||
|
|
||
| public void Clear() => cache.Clear(); | ||
| public void Clear() | ||
| { | ||
| defaultCache.Clear(); | ||
| versionedCache.Clear(); | ||
| } |
There was a problem hiding this comment.
The new versioned caches (e.g., versionedCache) are keyed by CacheKey and are never pruned. If callers toggle config (MaxRecursionDepth / type behaviors) repeatedly, these dictionaries can grow without bound and retain all historical entries. Consider a cleanup strategy (e.g., clear per-config caches when publishing a new config, keep only the latest N cacheKeys, or use a ConditionalWeakTable keyed by config instances).
| private static readonly ConcurrentDictionary<CacheIdentity, CacheEntry> versionedEntries = new(); | ||
|
|
||
| public static CacheEntry GetCurrent() | ||
| public static CacheEntry GetCurrent(FastClonerRuntimeConfig config) | ||
| { | ||
| long currentVersion = FastClonerCache.GetCacheVersion(); | ||
|
|
||
| if (config.CacheKey != 0) | ||
| { | ||
| CacheIdentity identity = new(currentVersion, config.CacheKey); | ||
| return versionedEntries.GetOrAdd(identity, _ => Refresh(config, currentVersion)); | ||
| } |
There was a problem hiding this comment.
ClonerCache<T> now stores per-(clearVersion, configCacheKey) entries in versionedEntries, but this dictionary is never cleared when cacheVersion bumps or when configs change. Over time this can accumulate entries for every config version ever published. Consider clearing versionedEntries when the global cache version changes, or otherwise bounding/pruning old identities.
| private static readonly ConcurrentDictionary<Type, bool> defaultKnownTypes = []; | ||
| private static readonly ConcurrentDictionary<ConfigKnownTypeKey, bool> versionedKnownTypes = []; | ||
|
|
||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| private static long GetAmbientCacheKey() | ||
| { | ||
| FastClonerRuntimeConfig? current = FastClonerRuntimeConfigScope.TryGetCurrent(); | ||
| return current?.CacheKey ?? FastCloner.GetPublishedCacheKey(); | ||
| } |
There was a problem hiding this comment.
FastClonerSafeTypes caches known-type results in versionedKnownTypes keyed by config CacheKey, but this cache is never pruned. If config cache keys are frequently incremented, this can grow indefinitely. Consider clearing versioned known-types when publishing a new config (or bounding/pruning old cache keys).
|
|
||
| [Test] | ||
| public void Nullable_Shoild_Be_Cloned() | ||
| public async Task Nullable_Shoild_Be_Cloned() |
There was a problem hiding this comment.
Typo in the test name: Nullable_Shoild_Be_Cloned → Nullable_Should_Be_Cloned (helps discoverability/searchability and keeps naming consistent with other tests).
| public async Task Nullable_Shoild_Be_Cloned() | |
| public async Task Nullable_Should_Be_Cloned() |
| publishedEngine = CreatePublishedEngine( | ||
| current.MaxRecursionDepth, | ||
| value, | ||
| current.CloneTypeBehaviors()); | ||
| } |
There was a problem hiding this comment.
publishedEngine is read via Volatile.Read(ref publishedEngine) but written with a plain assignment inside the lock. Readers don't take the lock, so this isn't a guaranteed safe publication on all architectures. Use Volatile.Write(ref publishedEngine, ...) or Interlocked.Exchange(ref publishedEngine, ...) for the assignment(s) to match the volatile read semantics.
| /// <remarks> | ||
| /// Setting <see cref="CloneBehavior.Clone"/> removes any custom behavior (equivalent to <see cref="ClearTypeBehavior"/>). | ||
| /// Note that changing behavior clears the cache, which may impact performance until the cache is repopulated. | ||
| /// </remarks> |
There was a problem hiding this comment.
The XML docs for SetTypeBehavior / ClearTypeBehavior / ClearAllTypeBehaviors state that changing behavior clears the cache, but the implementation now only republishes a new runtime config without calling FastClonerCache.ClearCache() / FastClonerSafeTypes.ClearKnownTypesCache(). Either update the docs to reflect the new versioned-cache approach, or explicitly clear caches here if that contract is still intended.
Deep Clone Benchmarks
Current FastCloner vs DeepCloner
FastCloner vs latest
|
| Status | Benchmark | Delta Time | Delta Alloc |
|---|---|---|---|
| ⚪ | DynamicWithArray | -2% faster | ~same |
| ⚪ | DynamicWithDictionary | ~same | ~same |
| 🟢 | DynamicWithNestedObject | -6% faster | ~same |
| 🟢 | FileSpec | -14% faster | ~same |
| 🔴 | LargeEventDocument_10MB | +9% slower | ~same |
| ⚪ | LargeLogBatch_10MB | ~same | ~same |
| ⚪ | MediumNestedObject | ~same | ~same |
| ⚪ | ObjectDictionary_50 | ~same | ~same |
| ⚪ | ObjectList_100 | ~same | ~same |
| 🟢 | SmallObject | -6% faster | ~same |
| ⚪ | SmallObjectWithCollections | +4% slower | ~same |
| ⚪ | StringArray_1000 | -5% faster | ~same |
Regressions
LargeEventDocument_10MB: time +9% slower, alloc ~same
Improvements
DynamicWithNestedObject: time -6% faster, alloc ~sameFileSpec: time -14% faster, alloc ~sameSmallObject: time -6% faster, alloc ~same
Mixed changes
- none
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 53 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
src/FastCloner/Code/FastCloneState.cs:140
FastCloneStateinstances returned to the pool keep a reference to whateverConfigurationthey last used. If a non-defaultFastClonerRuntimeConfigwas used, this can retain type-behavior dictionaries and prevent them from being GC’d while the state sits in the pool. Consider resettingConfigurationback toFastClonerRuntimeConfig.DefaultinsideReset()(or just before pooling inReturn).
.github/workflows/benchmark.yml:201RESULT_DIRis written assrc/benchmark-results/...(relative to repo root), but the later “Generate normalized report and diff” step runs withworking-directory: srcand uses$env:RESULT_DIRdirectly. This will create outputs undersrc/src/benchmark-results/...and break the expected artifact paths. Either setRESULT_DIRtobenchmark-results/...when running undersrc, or dropworking-directory: srcfor the report/diff step.
- name: Generate normalized report and diff
if: steps.run_gate.outputs.should_run == 'true'
working-directory: src
shell: pwsh
run: |
New-Item -ItemType Directory -Force -Path $env:RESULT_DIR | Out-Null
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 55 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| env: | ||
| DOTNET_VERSION: "10.0.103" | ||
| BASELINE_BRANCH: "next" | ||
|
|
There was a problem hiding this comment.
The workflow sets BASELINE_BRANCH to next, but the benchmark CI README describes baseline artifacts and slow-path cloning coming from master. Please align the workflow/env var with the documented baseline branch (or update the README to match the intended branch).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 55 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/FastCloner/Code/FastClonerSafeTypes.cs:286
ClearKnownTypesCacheswaps out theknownTypesdictionary without any memory barrier. Because this cache is read concurrently, the assignment should useVolatile.Write/Interlocked.Exchange(and reads should useVolatile.Read) to avoid publication/reordering issues under concurrency when replacing the dictionary instance.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - Push/PR to `next`: runs on `ubuntu-latest` by default. | ||
| - Manual run (`workflow_dispatch`): OS checkboxes allow running on Windows, Ubuntu, and macOS. | ||
| - PR runs post an upserted comment with: | ||
| - current `FastCloner` vs `DeepCloner` table | ||
| - regression/improvement diff against latest successful baseline from `next` | ||
| - regression/improvement diff against the latest successful baseline from `master` | ||
| - Baseline resolution order for PR runs: | ||
| - use the latest successful uploaded baseline artifact for the target OS from `master` | ||
| - if no artifact is available, fall back to a slow path that clones `master`, runs the benchmark suite there, and generates a fresh normalized baseline before comparing |
There was a problem hiding this comment.
This README says PR benchmarks diff against a baseline from master, but .github/workflows/benchmark.yml is currently configured with BASELINE_BRANCH: "next". Update the README or the workflow so they agree on which branch provides the baseline artifacts/slow-path baseline generation.
| @@ -26,6 +26,9 @@ static FastClonerExprGenerator() | |||
| fieldSetMethod = typeof(FieldInfo).GetMethod(nameof(FieldInfo.SetValue), [typeof(object), typeof(object)])!; | |||
| } | |||
|
|
|||
| internal static void ClearAdaptiveDictionaryFactoryCache() => adaptiveDictionaryFactoryCache = new ConcurrentDictionary<AdaptiveDictionaryFactoryKey, Func<ExpressionPosition, object>>(); | |||
| internal static int GetAdaptiveDictionaryFactoryCacheCountForTesting() => adaptiveDictionaryFactoryCache.Count; | |||
|
|
|||
There was a problem hiding this comment.
ClearAdaptiveDictionaryFactoryCache replaces adaptiveDictionaryFactoryCache while other threads may be reading/adding. Use Volatile.Write/Interlocked.Exchange for the swap (and Volatile.Read at call sites if needed) to ensure safe publication of the new dictionary under concurrency.
| <ItemGroup> | ||
| <PackageReference Include="FluentNHibernate" Version="3.4.1" /> | ||
| <PackageReference Include="FluentValidation" Version="12.1.1" /> | ||
| <PackageReference Include="Microsoft.NET.Test.Sdk" Version="18.3.0" /> | ||
| <PackageReference Include="NHibernate" Version="5.6.0" /> | ||
| <PackageReference Include="NUnit" Version="4.5.0" /> | ||
| <PackageReference Include="NUnit3TestAdapter" Version="5.2.0" /> | ||
| <PackageReference Include="NUnit.Analyzers" Version="4.11.2"> | ||
| <PrivateAssets>all</PrivateAssets> | ||
| <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> | ||
| </PackageReference> | ||
| <PackageReference Include="ObjectDumper.NET" Version="4.4.10-pre" /> | ||
| <PackageReference Include="System.Collections.Immutable" Version="10.0.1" /> | ||
| <PackageReference Include="System.Data.SqlClient" Version="4.9.1" /> | ||
| <PackageReference Include="System.Data.SQLite.Core" Version="1.0.119" /> | ||
| <PackageReference Include="System.Drawing.Common" Version="10.0.1" /> | ||
| <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Analyzer.Testing" Version="1.1.3" /> | ||
| <PackageReference Include="Microsoft.CodeAnalysis.CSharp.CodeFix.Testing" Version="1.1.3" /> | ||
| <PackageReference Include="System.Threading.Tasks.Dataflow" Version="10.0.1" /> | ||
| <PackageReference Include="TUnit" Version="1.19.16" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
System.Threading.Tasks.Dataflow is used by FailureHypothesisTests (e.g., BufferBlock<T>), but the test project no longer references the System.Threading.Tasks.Dataflow NuGet package. This will break compilation unless the API is provided by the target framework. Add the package reference back (or remove/conditionalize the Dataflow-based tests).
No description provided.