Preserve safe collection fast paths while tracking storage cycles

[niemyjski]

Draft for approach review: collection keys, comparers, cached views, and storage can contain cycles despite acyclic values. Found during an agent-assisted Foundatio review, this fix preserves the payload cache and known-safe fast paths, restores tracking elsewhere, and removes an unnecessary large-array reservation.

Maintainer decision: retain this conditional policy or design an explicit caller policy? Small dictionary clones still cost about 9 ns more (~19%); larger dictionaries/sets add about 64 bytes. Set timings remain noisy; performance acceptance stays open.

Validation at d550f5a: 1,058 local tests passed, six skipped; all four library targets built without warnings/errors. Workflow files match the base branch; existing fork-token publication failures remain outside this PR.

Known limitations: small dictionaries can retain a custom comparer pointing to the original dictionary (also reproducible on the base). The no-tracking fast path is not claimed for .NET Framework implementations with SyncRoot fields.

Verification and implementation details

Why the original optimization exists

Blame traces the shortcut and payload cache to 32e799c, “runtime: perf, internalization builder”. It avoids repeated payload reflection and selects CyclePolicy.None, allowing the thread-static simple state to skip reference tracking on each clone. The original broad removal gave up useful runtime performance, not just a metadata lookup.

98b5f531, “improve cycles tracking”, subsequently added the struct-mediated self-reference guard. Both the payload cache and this guard are retained. The commit history does not establish an isolated benchmark benefit for this shortcut; the measurements below compare the actual revisions.

Narrower proposal and behavioral coverage

• Check the concrete shape before discovering or caching its payload type; unsupported shapes now bypass that lookup. Apply the payload shortcut only to exact List<T>, Queue<T>, and Stack<T>, still requiring safe/reference-free payloads and the existing guard. Derived types can add state and take the existing fallback.
• Keep arrays, safe types, metadata caches, and existing custom clone-behavior handling unchanged. There is no public API or dependency change.
• Use existing field-based analysis for other shapes. Dictionary value types do not describe key/comparer graphs. Even Dictionary<int,int> can have materialized Keys/Values views pointing back to it; the test uses 100 entries to exercise its memberwise path above the adaptive rebuild threshold. Sets can have comparers referring to their owner.
• Added tests exercise concurrent dictionary storage, cyclic keys, cached views, comparer ownership, custom collection storage, derived lists, mutation isolation, and the preserved fast-path classification. Tests guard the classifier before cloning so regressions fail without overflowing the test process. Existing FrozenSet and struct-mediated cycle tests remain intact.

Disabling metadata caching would not repair the inference. An explicit caller policy to omit tracking could be useful for known graphs, but needs defined cycle/shared-reference behavior and configuration-aware cached metadata. This draft leaves that API decision open rather than introducing a global unsafe opt-out.

Review findings and scope

The payload lookup was still running before the known-shape guard. d550f5a moves the guard ahead of it without removing the cache. A new regression verifies that analyzing an unsupported collection does not populate the payload cache; it fails before the fix and passes after it.

The small-dictionary comparer observation is valid but pre-existing: both exact parent c3fc2eb and the candidate share a custom comparer whose owner remains the original dictionary for the one-entry reproduction. This cycle-classification change does not repair the adaptive rebuild path's comparer handling and does not guarantee complete comparer isolation. A comparer fix must also preserve hash/lookup correctness and cyclic ownership; it is not folded into this metadata-lookup correction.

The .NET Framework observation also concerns existing behavior. Microsoft's List reference source, Queue, and Stack contain an object-typed _syncRoot field. The retained guard conservatively treats it as potentially pointing to the collection. The exact parent already uses that same guard, so this PR does not introduce the claimed regression. net46 compilation passed; runtime performance there remains unverified. No guard is removed merely to make an untested performance claim.

Allocation issue exposed by restoring tracking

Clone1DimArraySafeInternal reserved reference slots for every array element even though safe elements are not cloned or registered individually. Arrays above the reference pool's retention threshold repeatedly allocated that unused capacity. The first 10,000-item comparison showed roughly 3–4× allocation in the conditional proposal.

The follow-up removes that one reservation and retains AddKnownRef, which already grows storage when necessary. Four regressions cover integer/string arrays with zero/four prior tracked references, checking contents, reference identity, and bounded tracking capacity. This avoids discarding the useful cycle fix to recover performance.

Controlled performance comparison

BenchmarkDotNet 0.15.8 with MemoryDiagnoser; macOS arm64/M4 Max, .NET 10.0.12. Separate DLL jobs compare exact parent c3fc2eb, conditional proposal before the reservation fix 40c8f63, and runtime code 61ce97a. These measurements predate d550f5a; the new cold-metadata lookup optimization has not been separately benchmarked. Job DLL hashes were checked against inputs. Warm setup pre-clones every shape; payloads are integers. All 27 final Dry cases and 27 ShortRun measurements completed (three measured iterations per case). Stack/set timing outliers were repeated with ten iterations and two launches; those two table rows use the repeated means. These are limited local measurements, not an exhaustive performance guarantee.

Warm clone	Parent	Final	Parent → final allocation
List, 10	16.96 ns	16.34 ns	96 → 96 B
List, 1,000	160.38 ns	157.94 ns	4,056 → 4,056 B
Queue, 1,000	205.79 ns	180.82 ns	4,064 → 4,064 B
Stack, 1,000 (repeat)	184.5 ns	179.2 ns	4,056 → 4,056 B
Dictionary, 10	46.01 ns	54.83 ns	352 → 352 B
Dictionary, 1,000	679.63 ns	672.54 ns	22,192 → 22,256 B
HashSet, 1,000 (repeat)	690.5 ns	639.9 ns	17,768 → 17,832 B
Dictionary, 10,000	18.90 µs	18.69 µs	197.50 → 197.56 KB
HashSet, 10,000	15.12 µs	14.78 µs	158.01 → 158.08 KB

The repeated HashSet result remains variable: parent/final medians are 607.9/641.6 ns, while the means above run the other direction because of parent outliers. This does not establish a speedup or rule out a smaller regression.

Before the reservation correction, the 10,000-item dictionary allocated 645.77 KB and the set 606.28 KB per clone. Retaining reference tracking does not require those buffers. In the original three-way comparison, broad shortcut removal made List<int> with ten items 26.26 ns versus the parent's 16.89 ns; retaining its fast path avoids that cost.

The first conditional run had a 2× large-dictionary timing outlier that did not reproduce with ten iterations and two launches. Cold-cache measurements on the initial conditional candidate were noisy and do not support a cold-start improvement claim. The earlier hosted report compared FastCloner/DeepCloner ratios against a saved baseline from another run; its percentages are not isolated patch effects.

Validation and existing CI limitation

Runtime validation at 61ce97a passed: Windows and Linux each 1,059 passed / four skipped; macOS 1,057 passed / six skipped. Both compatibility builds passed. That run precedes the workflow revert and the latest payload-lookup change; it is historical validation, not a claim that current overall checks pass.

Earlier build/test run compiled net46/netstandard2.0 and passed Linux tests before fork-restricted status writes failed with Resource not accessible by integration; matrix fail-fast canceled other platforms. Earlier benchmark run completed execution, reports, and artifact upload before comment publication failed with HTTP 403.

Workflow edits have been removed from this PR. All workflow files match the base branch. Those existing publication failures remain outside the cloning fix and may make overall CI checks fail despite successful build/test steps.

dotnet run -c Release --project src/FastCloner.Tests -- --treenode-filter '/*/*/CollectionTests/*'
dotnet run -c Release --no-build --project src/FastCloner.Tests
dotnet build src/FastCloner/FastCloner.csproj -c Release --no-restore

The four library targets are net10.0, net8.0, netstandard2.0, and net46. Compilation is not runtime verification on every target. The additional .NET 8 probe checks list/queue/stack isolation, dictionary views/keys, set comparer ownership, and concurrent dictionary storage/comparer behavior. Nine collection regressions fail against the exact parent, and four allocation regressions fail against the pre-reservation-fix library. Six existing local skips remain. Foundatio's vendored workaround has been removed; this draft is independent, with no release or dependency update bundled here.

[Copilot]

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified.

Review effort: Lite
Findings: None

What changed in this PR

Updates cycle detection to preserve reference tracking for collection storage and dictionary keys during deep cloning.

Changes:

• Removes the collection payload shortcut and obsolete cache/helpers.
• Adds regressions for internal collection cycles and cyclic dictionary keys.

File	Description
src/​FastCloner/​Code/​FastClonerGenerator.cs	Uses field-based cycle analysis for collections.
src/​FastCloner/​Code/​FastClonerCache.cs	Removes obsolete payload-type caching.
src/​FastCloner.Tests/​CollectionTests.cs	Adds cycle-tracking regression tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[niemyjski]

The revised draft keeps the payload cache and the existing struct-mediated guard. Blame traces the shortcut to the March runtime performance work and the guard to the May cycle-tracking fix; removing the shortcut broadly gave up a useful fast path (the small-list benchmark went from about 17 ns to 26 ns).

The current proposal limits payload-only inference to exact List/Queue/Stack shapes and uses existing field-based tracking elsewhere. Larger benchmarks also exposed a separate over-reservation in safe-array tracking: it reserved one slot per element despite registering only the array. Removing that single reservation eliminates the roughly 3–4× allocation increase at 10,000 dictionary/set entries while preserving reference tracking.

The runtime code passed Windows/Linux/macOS builds and tests at 61ce97a. Workflow edits have since been removed; all workflow files match the base branch, and existing fork-token publication failures remain outside this PR. The description contains the exact-parent measurements and remaining tradeoffs: small dictionaries cost about 9 ns more (~19%), larger hash collections add about 64 bytes, and set timings remain noisy.

Would you prefer this conditional boundary, a different set of proven collection shapes, or an explicit caller graph policy? A metadata-cache toggle alone would not fix the cycle inference. Keeping this draft for that design/performance discussion; no new public setting is imposed.

[Copilot]

Copilot review overview

🔵 Needs a closer look

A moderate implementation issue remains, and performance acceptance is unresolved.

Review effort: Lite
Findings: 1

Open (1)

• Avoid payload lookup for unsupported collection shapes · New

[Copilot]

Copilot review overview

🔵 Needs a closer look

The safe dictionary path can retain a custom comparer referencing the original dictionary.

Review effort: Lite
Findings: 1

Open (1)

• Avoid payload lookup for unsupported collection shapes

[Copilot]

Copilot review overview

🔵 Needs a closer look

The net46 target does not preserve the intended fast path and needs correction or documented coverage.

Review effort: Lite
Findings: 1

Open (1)

• Avoid payload lookup for unsupported collection shapes

Previously missed (1)

In code that hasn't changed since last review

Preserve net46 fast path despite ICollection.SyncRoot field

src/​FastCloner/​Code/​FastClonerGenerator.cs:478

This still does not preserve the fast path on the net46 target: the framework List<T>, Queue<T>, and Stack<T> implementations have an ICollection.SyncRoot reference field, and ContainerHasStructMediatedSelfReference treats that object field as potentially referencing the root. Consequently this branch returns true for the reference-free payloads on that supported target, so cloning continues to use tracked state there; please account for the target-specific field or add coverage and document the intentional regression.

        if (payloadType is not null && IsKnownArrayBackedCollection(type))
        {
            bool payloadTriviallyAcyclic =
                FastClonerSafeTypes.CanReturnSameObject(payloadType) ||
                (payloadType.IsValueType && !ValueTypeContainsReferenceFieldsCached(payloadType));

            if (payloadTriviallyAcyclic)
            {
                return ContainerHasStructMediatedSelfReference(type);

[Copilot]

Copilot review overview

🔵 Needs a closer look

The performance-sensitive cycle-tracking changes warrant final human review.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Avoid payload lookup for unsupported collection shapes