From 0ec50b55d42b3a026e80dd77d4be704779c3e244 Mon Sep 17 00:00:00 2001 From: Max Charlamb Date: Sun, 21 Jun 2026 20:36:44 -0400 Subject: [PATCH 1/3] [cdac] Cache LayoutSet resolution via host-internal IGeneratedType contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cdac source generator emits an `IData` constructor for every data class that calls `LayoutSet.Resolve(target, _typeNames)` to discover the field layout. `Resolve` allocates a `List` plus a backing array on every invocation, even though the result is a function of (Target, _typeNames) — both stable for the lifetime of a target snapshot. On a heap walk that constructs 1M `Data.Array` instances this cost is ~340 MB of throwaway allocations. This change caches the resolved `LayoutSet` per (Target, _typeNames): - Adds a fallback registration version `""` (empty string) to `ContractRegistry.Register`. The registry consults the target's contract descriptor first; if no version is advertised (or no implementation is registered for the advertised version), it falls back to the empty-string default. This lets host-side helpers — caches, adapters, cross-contract aggregators — register implementations the target doesn't need to advertise, while still permitting a future target to take over a contract via descriptor advertisement. - Introduces a new `IGeneratedType` contract emitted by the source generator (alongside `LayoutSet` and `TypeNameResolver`) whose single implementation `GeneratedType_1` caches `LayoutSet.Resolve` results keyed by reference identity on the static `_typeNames` arrays the generator already emits per data class. - Adds a `TargetExtensions.GetCachedLayoutSet` extension method (also generator-emitted) that performs lazy first-use registration of `IGeneratedType` and dispatches the cached lookup. - Switches `Emitter` to emit `_target.GetCachedLayoutSet(_typeNames)` instead of `LayoutSet.Resolve(_target, _typeNames)`. - Changes `LayoutSet` from `readonly struct` to `sealed class` so the cached entry is shared by reference rather than copied on every Dictionary lookup and method dispatch — eliminates JIT defensive copies and saves 8 bytes per fetch. - Updates the two test `ContractRegistry` stubs to honor the same empty-version fallback rule. Benchmark on a 1M small-string heap walk (medians of 10 iterations, standalone HeapWalkBench tool not included in this commit): Variant | ms before | ms after | KB before | KB after CachedIData | 724 | 461 | 364,571 | 43,545 UncachedIData | 338 | 281 | 364,571 | 43,532 RawRead | 174 | 175 | 4,536 | 4,389 CachedIData -36% time, -88% allocations UncachedIData -17% time, -88% allocations RawRead unchanged (control) Combined with the LinearReadCache scope API in #129345, the UncachedIData variant drops further to 226 ms / 4,389 KB — matching RawRead's allocation profile entirely, as JIT escape analysis is then able to stack-allocate the per-object `Data.Array` instances. All 2,605 cdac unit + DataGenerator tests pass on a clean rebuild. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../ContractRegistry.cs | 10 ++ .../CachingContractRegistry.cs | 21 ++- src/native/managed/cdac/gen/CdacGenerator.cs | 12 +- src/native/managed/cdac/gen/Emitter.cs | 4 +- .../cdac/gen/GeneratedTypeContractSource.cs | 121 ++++++++++++++++++ .../managed/cdac/gen/LayoutSetSource.cs | 2 +- .../cdac/tests/DataGenerator/TestTarget.cs | 26 +++- .../TestPlaceholderTarget.cs | 18 ++- 8 files changed, 200 insertions(+), 14 deletions(-) create mode 100644 src/native/managed/cdac/gen/GeneratedTypeContractSource.cs diff --git a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractRegistry.cs b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractRegistry.cs index 76b7ca71eb0537..078a6b87b7bd0c 100644 --- a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractRegistry.cs +++ b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractRegistry.cs @@ -171,6 +171,16 @@ public bool TryGetContract([NotNullWhen(true)] out TContract contract /// Register a contract implementation for a specific version. /// External packages use this to add contract versions or entirely new contract interfaces. /// + /// + /// The empty string ("") is a reserved "default" version. If a target's + /// contract descriptor does not declare a version for the contract — or + /// declares a version that is not registered — the registry falls back to the + /// implementation registered with the empty-string version, if any. This + /// allows host-side helpers (caches, adapters, cross-contract aggregators) + /// to provide an implementation that the target doesn't need to advertise, + /// while still allowing a future target to take over the contract by + /// declaring an explicit version. + /// public abstract void Register(string version, Func creator) where TContract : IContract; diff --git a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/CachingContractRegistry.cs b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/CachingContractRegistry.cs index 376b680f35966c..c3b21cb976cfa3 100644 --- a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/CachingContractRegistry.cs +++ b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/CachingContractRegistry.cs @@ -48,15 +48,26 @@ public override bool TryGetContract([NotNullWhen(true)] out TContract return true; } - if (!_tryGetContractVersion(TContract.Name, out string? version)) + Func? creator = null; + if (_tryGetContractVersion(TContract.Name, out string? version) + && _creators.TryGetValue((typeof(TContract), version), out Func? versioned)) { - failureReason = $"Target does not support contract '{typeof(TContract).Name}'."; - return false; + creator = versioned; + } + else if (_creators.TryGetValue((typeof(TContract), string.Empty), out Func? fallback)) + { + // No target-declared version, or version present but no registered + // implementation for it. Fall back to the empty-string "default" + // registration if any caller has provided one (e.g. host-side + // helpers like the layout-set cache). + creator = fallback; } - if (!_creators.TryGetValue((typeof(TContract), version), out Func? creator)) + if (creator is null) { - failureReason = $"Target supports contract '{typeof(TContract).Name}' version {version}, but no implementation is registered for that version."; + failureReason = version is null + ? $"Target does not support contract '{typeof(TContract).Name}'." + : $"Target supports contract '{typeof(TContract).Name}' version {version}, but no implementation is registered for that version."; return false; } diff --git a/src/native/managed/cdac/gen/CdacGenerator.cs b/src/native/managed/cdac/gen/CdacGenerator.cs index 2375b2fdb60d05..521f99751a1ac0 100644 --- a/src/native/managed/cdac/gen/CdacGenerator.cs +++ b/src/native/managed/cdac/gen/CdacGenerator.cs @@ -34,10 +34,11 @@ public void Initialize(IncrementalGeneratorInitializationContext context) // sees Contracts' copy via [InternalsVisibleTo] and shouldn't emit its own). // Each helper is gated independently to handle version-skew scenarios where // one helper is present but the other is not. - IncrementalValueProvider<(bool EmitLayoutSet, bool EmitTypeNameResolver)> shouldEmitHelpers = context.CompilationProvider + IncrementalValueProvider<(bool EmitLayoutSet, bool EmitTypeNameResolver, bool EmitGeneratedTypeContract)> shouldEmitHelpers = context.CompilationProvider .Select(static (compilation, _) => ( EmitLayoutSet: !IsTypeAccessible(compilation, LayoutSetSource.FullyQualifiedName), - EmitTypeNameResolver: !IsTypeAccessible(compilation, TypeNameResolverSource.FullyQualifiedName))); + EmitTypeNameResolver: !IsTypeAccessible(compilation, TypeNameResolverSource.FullyQualifiedName), + EmitGeneratedTypeContract: !IsTypeAccessible(compilation, GeneratedTypeContractSource.FullyQualifiedName))); context.RegisterSourceOutput(shouldEmitHelpers, static (ctx, flags) => { @@ -54,6 +55,13 @@ public void Initialize(IncrementalGeneratorInitializationContext context) TypeNameResolverSource.HintName, SourceText.From(TypeNameResolverSource.Source, Encoding.UTF8)); } + + if (flags.EmitGeneratedTypeContract) + { + ctx.AddSource( + GeneratedTypeContractSource.HintName, + SourceText.From(GeneratedTypeContractSource.Source, Encoding.UTF8)); + } }); IncrementalValuesProvider models = context.SyntaxProvider diff --git a/src/native/managed/cdac/gen/Emitter.cs b/src/native/managed/cdac/gen/Emitter.cs index 162d75d2ddcce4..920346ef18af87 100644 --- a/src/native/managed/cdac/gen/Emitter.cs +++ b/src/native/managed/cdac/gen/Emitter.cs @@ -134,7 +134,7 @@ private static void EmitWriteBackMethod(StringBuilder sb, MemberModel member) sb.AppendLine($" public void Write{member.Name}({propType} value)"); sb.AppendLine(" {"); - sb.AppendLine($" LayoutSet layouts = LayoutSet.Resolve(_target, _typeNames);"); + sb.AppendLine($" LayoutSet layouts = _target.GetCachedLayoutSet(_typeNames);"); sb.AppendLine($" layouts.Select(Address, out var t, out var b, out var n, {NameArgs(member)});"); if (member.ReadKind == FieldReadKind.Bool) { @@ -176,7 +176,7 @@ private static void EmitConstructor(StringBuilder sb, CdacTypeModel model, bool if (needsDescriptor) { sb.AppendLine(); - sb.AppendLine($" LayoutSet layouts = LayoutSet.Resolve(target, _typeNames);"); + sb.AppendLine($" LayoutSet layouts = target.GetCachedLayoutSet(_typeNames);"); } sb.AppendLine(); diff --git a/src/native/managed/cdac/gen/GeneratedTypeContractSource.cs b/src/native/managed/cdac/gen/GeneratedTypeContractSource.cs new file mode 100644 index 00000000000000..91f2f0557f286e --- /dev/null +++ b/src/native/managed/cdac/gen/GeneratedTypeContractSource.cs @@ -0,0 +1,121 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.Diagnostics.DataContractReader.DataGenerator; + +/// +/// Source for the IGeneratedType contract, its GeneratedType_1 +/// implementation, and the TargetExtensions.GetCachedLayoutSet helper, +/// all emitted into each consuming assembly via RegisterSourceOutput +/// (gated by CompilationProvider to avoid duplicate symbols). +/// +/// +/// The contract caches the result of LayoutSet.Resolve(target, _typeNames) +/// keyed by reference identity on the _typeNames array. The generator +/// emits exactly one static _typeNames per IData class, so reference +/// equality is the correct comparer and yields a 100% hit rate after the +/// first instantiation of each data class. +/// +/// Registration happens lazily inside GetCachedLayoutSet: the first +/// call on a given registers the +/// implementation; subsequent calls hit the cached contract instance. +/// Re-registering with the same lambda matches the no-op semantics of +/// 's last-write-wins +/// behavior, so concurrent lazy registrations from multiple call sites are +/// safe. +/// +internal static class GeneratedTypeContractSource +{ + public const string HintName = "GeneratedTypeContract.g.cs"; + + public const string Namespace = "Microsoft.Diagnostics.DataContractReader.Generated"; + + public const string FullyQualifiedName = Namespace + ".IGeneratedType"; + + public const string Source = """ +// +#nullable enable + +using System; +using System.Collections.Generic; +using Microsoft.Diagnostics.DataContractReader; +using Microsoft.Diagnostics.DataContractReader.Contracts; + +namespace Microsoft.Diagnostics.DataContractReader.Generated; + +/// +/// Diagnostics-host-internal contract that caches per-target results of +/// , keyed by reference +/// identity on the generator-emitted static _typeNames array of each +/// IData class. Has no target-side data and is not advertised by the target's +/// contract descriptor; registered under +/// with the empty-string +/// "default" version, which the registry falls back to when no target-declared +/// version exists. +/// +internal interface IGeneratedType : IContract +{ + static string IContract.Name => nameof(GeneratedType); + + LayoutSet GetOrAddLayoutSet(string[] typeNames); +} + +/// +/// Marker type whose nameof supplies the contract name. Internal +/// contracts are not advertised in the target descriptor so this value is +/// purely diagnostic. +/// +internal static class GeneratedType { } + +internal sealed class GeneratedType_1 : IGeneratedType +{ + private readonly Target _target; + private readonly Dictionary _cache = + new(ReferenceEqualityComparer.Instance); + + public GeneratedType_1(Target target) => _target = target; + + public LayoutSet GetOrAddLayoutSet(string[] typeNames) + { + if (!_cache.TryGetValue(typeNames, out LayoutSet? cached)) + { + cached = LayoutSet.Resolve(_target, typeNames); + _cache[typeNames] = cached; + } + return cached; + } + + public void Flush(FlushScope scope) + { + // LayoutSets resolve type info from contract descriptors and managed + // metadata, both stable for the lifetime of a target snapshot and + // immutable across FlushScope.ForwardExecution. Only clear on + // FlushScope.All (matches a target re-attach / arbitrary snapshot). + if (scope == FlushScope.All) + _cache.Clear(); + } +} + +/// +/// Extension helpers for code generated by the cdac source generator. Bridges +/// the generated IData constructors to the internal +/// cache contract, performing first-use lazy registration so no global +/// registration step is required. +/// +internal static class TargetExtensions +{ + public static LayoutSet GetCachedLayoutSet(this Target target, string[] typeNames) + { + if (!target.Contracts.TryGetContract(out IGeneratedType contract)) + { + // Register under the empty-string "default" version. The target + // descriptor does not advertise IGeneratedType (it has no + // target-side data), so the registry falls back to this entry. + target.Contracts.Register(string.Empty, static t => new GeneratedType_1(t)); + contract = target.Contracts.GetContract(); + } + return contract.GetOrAddLayoutSet(typeNames); + } +} +"""; +} diff --git a/src/native/managed/cdac/gen/LayoutSetSource.cs b/src/native/managed/cdac/gen/LayoutSetSource.cs index d7e7ac962a1997..b910d95ee56dfb 100644 --- a/src/native/managed/cdac/gen/LayoutSetSource.cs +++ b/src/native/managed/cdac/gen/LayoutSetSource.cs @@ -33,7 +33,7 @@ namespace Microsoft.Diagnostics.DataContractReader.Generated; /// first, then managed type metadata), trying each candidate field name /// per source. Sources are resolved lazily. /// -internal readonly struct LayoutSet +internal sealed class LayoutSet { private readonly LazyLayout[] _layouts; diff --git a/src/native/managed/cdac/tests/DataGenerator/TestTarget.cs b/src/native/managed/cdac/tests/DataGenerator/TestTarget.cs index b823054d3b0220..a959d7381b9ce6 100644 --- a/src/native/managed/cdac/tests/DataGenerator/TestTarget.cs +++ b/src/native/managed/cdac/tests/DataGenerator/TestTarget.cs @@ -36,6 +36,7 @@ public TestTarget(int pointerSize = 8, bool isLittleEndian = true) IsLittleEndian = isLittleEndian; ManagedTypeSourceMock = new Mock(); _contracts = new TestContractRegistry(ManagedTypeSourceMock.Object); + _contracts.SetTarget(this); _processedData = new TestDataCache(this); } @@ -262,12 +263,17 @@ public override bool TryReadGlobalPointer(string name, [NotNullWhen(true)] out T private sealed class TestContractRegistry : ContractRegistry { private readonly IManagedTypeSource _managedTypeSource; + private readonly Dictionary<(Type, string), Func> _creators = new(); + private readonly Dictionary _resolved = new(); + private Target? _target; public TestContractRegistry(IManagedTypeSource managedTypeSource) { _managedTypeSource = managedTypeSource; } + public void SetTarget(Target target) => _target = target; + public override IManagedTypeSource ManagedTypeSource => _managedTypeSource; public override bool TryGetContract([NotNullWhen(true)] out TContract contract, out string? failureReason) @@ -278,13 +284,31 @@ public override bool TryGetContract([NotNullWhen(true)] out TContract failureReason = null; return true; } + if (_resolved.TryGetValue(typeof(TContract), out IContract? cached)) + { + contract = (TContract)cached; + failureReason = null; + return true; + } + // No target-declared versions in this stub — fall through directly + // to the empty-string "default" registration. + if (_creators.TryGetValue((typeof(TContract), string.Empty), out Func? fallback)) + { + if (_target is null) + throw new InvalidOperationException("TestContractRegistry: SetTarget must be called before TryGetContract."); + IContract created = fallback(_target); + _resolved[typeof(TContract)] = created; + contract = (TContract)created; + failureReason = null; + return true; + } contract = default!; failureReason = "Not registered in TestContractRegistry."; return false; } public override void Register(string version, Func creator) - => throw new NotImplementedException(); + => _creators[(typeof(TContract), version)] = t => creator(t); public override void Flush(FlushScope scope) { } } diff --git a/src/native/managed/cdac/tests/TestInfrastructure/TestPlaceholderTarget.cs b/src/native/managed/cdac/tests/TestInfrastructure/TestPlaceholderTarget.cs index 12561ffd8676a7..c427c1bd194276 100644 --- a/src/native/managed/cdac/tests/TestInfrastructure/TestPlaceholderTarget.cs +++ b/src/native/managed/cdac/tests/TestInfrastructure/TestPlaceholderTarget.cs @@ -635,13 +635,25 @@ public override bool TryGetContract([NotNullWhen(true)] out TContract } else if (_versions.TryGetValue(typeof(TContract), out string? version)) { - if (!_creators.TryGetValue((typeof(TContract), version), out var creator)) + if (_creators.TryGetValue((typeof(TContract), version), out var creator)) + { + resolved = creator(_target); + } + else if (_creators.TryGetValue((typeof(TContract), string.Empty), out var fallback)) + { + resolved = fallback(_target); + } + else { failureReason = $"Target supports contract '{typeof(TContract).Name}' version {version}, but no implementation is registered for that version."; return false; } - - resolved = creator(_target); + } + else if (_creators.TryGetValue((typeof(TContract), string.Empty), out var fallback)) + { + // No target-declared version — fall back to the empty-string + // "default" registration. Matches CachingContractRegistry. + resolved = fallback(_target); } else { From ab9fc6272eed966c55d9ab88622250e2854c44a5 Mon Sep 17 00:00:00 2001 From: Max Charlamb Date: Sun, 21 Jun 2026 20:54:18 -0400 Subject: [PATCH 2/3] Rename IGeneratedType -> IGeneratedTypeCache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The name "IGeneratedType" was ambiguous (a contract about generated types vs a contract holding generator-emitted state). "IGeneratedTypeCache" makes the intent — caching the type-lookup work the source generator performs — explicit. Renamed: IGeneratedType -> IGeneratedTypeCache GeneratedType (marker)-> GeneratedTypeCache GeneratedType_1 -> GeneratedTypeCache_1 GeneratedTypeContractSource.cs -> GeneratedTypeCacheContractSource.cs HintName "GeneratedTypeContract.g.cs" -> "GeneratedTypeCacheContract.g.cs" No behavior change. All 2,605 cdac unit + DataGenerator tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/native/managed/cdac/gen/CdacGenerator.cs | 10 ++--- ...cs => GeneratedTypeCacheContractSource.cs} | 43 ++++++++++--------- 2 files changed, 27 insertions(+), 26 deletions(-) rename src/native/managed/cdac/gen/{GeneratedTypeContractSource.cs => GeneratedTypeCacheContractSource.cs} (73%) diff --git a/src/native/managed/cdac/gen/CdacGenerator.cs b/src/native/managed/cdac/gen/CdacGenerator.cs index 521f99751a1ac0..1ccbe542f76db4 100644 --- a/src/native/managed/cdac/gen/CdacGenerator.cs +++ b/src/native/managed/cdac/gen/CdacGenerator.cs @@ -34,11 +34,11 @@ public void Initialize(IncrementalGeneratorInitializationContext context) // sees Contracts' copy via [InternalsVisibleTo] and shouldn't emit its own). // Each helper is gated independently to handle version-skew scenarios where // one helper is present but the other is not. - IncrementalValueProvider<(bool EmitLayoutSet, bool EmitTypeNameResolver, bool EmitGeneratedTypeContract)> shouldEmitHelpers = context.CompilationProvider + IncrementalValueProvider<(bool EmitLayoutSet, bool EmitTypeNameResolver, bool EmitGeneratedTypeCacheContract)> shouldEmitHelpers = context.CompilationProvider .Select(static (compilation, _) => ( EmitLayoutSet: !IsTypeAccessible(compilation, LayoutSetSource.FullyQualifiedName), EmitTypeNameResolver: !IsTypeAccessible(compilation, TypeNameResolverSource.FullyQualifiedName), - EmitGeneratedTypeContract: !IsTypeAccessible(compilation, GeneratedTypeContractSource.FullyQualifiedName))); + EmitGeneratedTypeCacheContract: !IsTypeAccessible(compilation, GeneratedTypeCacheContractSource.FullyQualifiedName))); context.RegisterSourceOutput(shouldEmitHelpers, static (ctx, flags) => { @@ -56,11 +56,11 @@ public void Initialize(IncrementalGeneratorInitializationContext context) SourceText.From(TypeNameResolverSource.Source, Encoding.UTF8)); } - if (flags.EmitGeneratedTypeContract) + if (flags.EmitGeneratedTypeCacheContract) { ctx.AddSource( - GeneratedTypeContractSource.HintName, - SourceText.From(GeneratedTypeContractSource.Source, Encoding.UTF8)); + GeneratedTypeCacheContractSource.HintName, + SourceText.From(GeneratedTypeCacheContractSource.Source, Encoding.UTF8)); } }); diff --git a/src/native/managed/cdac/gen/GeneratedTypeContractSource.cs b/src/native/managed/cdac/gen/GeneratedTypeCacheContractSource.cs similarity index 73% rename from src/native/managed/cdac/gen/GeneratedTypeContractSource.cs rename to src/native/managed/cdac/gen/GeneratedTypeCacheContractSource.cs index 91f2f0557f286e..c378a67d931d19 100644 --- a/src/native/managed/cdac/gen/GeneratedTypeContractSource.cs +++ b/src/native/managed/cdac/gen/GeneratedTypeCacheContractSource.cs @@ -4,10 +4,11 @@ namespace Microsoft.Diagnostics.DataContractReader.DataGenerator; /// -/// Source for the IGeneratedType contract, its GeneratedType_1 -/// implementation, and the TargetExtensions.GetCachedLayoutSet helper, -/// all emitted into each consuming assembly via RegisterSourceOutput -/// (gated by CompilationProvider to avoid duplicate symbols). +/// Source for the IGeneratedTypeCache contract, its +/// GeneratedTypeCache_1 implementation, and the +/// TargetExtensions.GetCachedLayoutSet helper, all emitted into each +/// consuming assembly via RegisterSourceOutput (gated by +/// CompilationProvider to avoid duplicate symbols). /// /// /// The contract caches the result of LayoutSet.Resolve(target, _typeNames) @@ -24,13 +25,13 @@ namespace Microsoft.Diagnostics.DataContractReader.DataGenerator; /// behavior, so concurrent lazy registrations from multiple call sites are /// safe. /// -internal static class GeneratedTypeContractSource +internal static class GeneratedTypeCacheContractSource { - public const string HintName = "GeneratedTypeContract.g.cs"; + public const string HintName = "GeneratedTypeCacheContract.g.cs"; public const string Namespace = "Microsoft.Diagnostics.DataContractReader.Generated"; - public const string FullyQualifiedName = Namespace + ".IGeneratedType"; + public const string FullyQualifiedName = Namespace + ".IGeneratedTypeCache"; public const string Source = """ // @@ -44,7 +45,7 @@ internal static class GeneratedTypeContractSource namespace Microsoft.Diagnostics.DataContractReader.Generated; /// -/// Diagnostics-host-internal contract that caches per-target results of +/// Target-agnostic contract that caches per-target results of /// , keyed by reference /// identity on the generator-emitted static _typeNames array of each /// IData class. Has no target-side data and is not advertised by the target's @@ -53,27 +54,27 @@ namespace Microsoft.Diagnostics.DataContractReader.Generated; /// "default" version, which the registry falls back to when no target-declared /// version exists. /// -internal interface IGeneratedType : IContract +internal interface IGeneratedTypeCache : IContract { - static string IContract.Name => nameof(GeneratedType); + static string IContract.Name => nameof(GeneratedTypeCache); LayoutSet GetOrAddLayoutSet(string[] typeNames); } /// -/// Marker type whose nameof supplies the contract name. Internal +/// Marker type whose nameof supplies the contract name. Target-agnostic /// contracts are not advertised in the target descriptor so this value is /// purely diagnostic. /// -internal static class GeneratedType { } +internal static class GeneratedTypeCache { } -internal sealed class GeneratedType_1 : IGeneratedType +internal sealed class GeneratedTypeCache_1 : IGeneratedTypeCache { private readonly Target _target; private readonly Dictionary _cache = new(ReferenceEqualityComparer.Instance); - public GeneratedType_1(Target target) => _target = target; + public GeneratedTypeCache_1(Target target) => _target = target; public LayoutSet GetOrAddLayoutSet(string[] typeNames) { @@ -98,21 +99,21 @@ public void Flush(FlushScope scope) /// /// Extension helpers for code generated by the cdac source generator. Bridges -/// the generated IData constructors to the internal -/// cache contract, performing first-use lazy registration so no global -/// registration step is required. +/// the generated IData constructors to the +/// contract, performing first-use lazy registration so no global registration +/// step is required. /// internal static class TargetExtensions { public static LayoutSet GetCachedLayoutSet(this Target target, string[] typeNames) { - if (!target.Contracts.TryGetContract(out IGeneratedType contract)) + if (!target.Contracts.TryGetContract(out IGeneratedTypeCache contract)) { // Register under the empty-string "default" version. The target - // descriptor does not advertise IGeneratedType (it has no + // descriptor does not advertise IGeneratedTypeCache (it has no // target-side data), so the registry falls back to this entry. - target.Contracts.Register(string.Empty, static t => new GeneratedType_1(t)); - contract = target.Contracts.GetContract(); + target.Contracts.Register(string.Empty, static t => new GeneratedTypeCache_1(t)); + contract = target.Contracts.GetContract(); } return contract.GetOrAddLayoutSet(typeNames); } From f7a4699bd57e6fc5844ed2892ae87cffe9f03b00 Mon Sep 17 00:00:00 2001 From: Max Charlamb Date: Sun, 21 Jun 2026 22:04:39 -0400 Subject: [PATCH 3/3] Address PR review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Restrict empty-string fallback to "no target-declared version" case only. If the target advertises a version but no implementation is registered for it, fail-fast as before — preserves version-skew detection. Applied to both CachingContractRegistry and the TestPlaceholderTarget stub. (Copilot review comment 1.) - Trim verbose doc comments on ContractRegistry.Register, GeneratedTypeCacheContractSource, and supporting types. Drops the unintended thread-safety implication noted by Copilot review and reduces overall comment volume. (Copilot review comment 3.) (Copilot review comment 2 about forwarding Flush in TestPlaceholderTarget was not addressed — the test stub does not exercise Flush regardless.) All 2,605 cdac unit + DataGenerator tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../ContractRegistry.cs | 17 ++----- .../CachingContractRegistry.cs | 30 +++++------ .../gen/GeneratedTypeCacheContractSource.cs | 51 ++----------------- .../TestPlaceholderTarget.cs | 16 ++---- 4 files changed, 27 insertions(+), 87 deletions(-) diff --git a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractRegistry.cs b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractRegistry.cs index 078a6b87b7bd0c..53fefbc8ea812e 100644 --- a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractRegistry.cs +++ b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractRegistry.cs @@ -168,19 +168,10 @@ public bool TryGetContract([NotNullWhen(true)] out TContract contract } /// - /// Register a contract implementation for a specific version. - /// External packages use this to add contract versions or entirely new contract interfaces. - /// - /// - /// The empty string ("") is a reserved "default" version. If a target's - /// contract descriptor does not declare a version for the contract — or - /// declares a version that is not registered — the registry falls back to the - /// implementation registered with the empty-string version, if any. This - /// allows host-side helpers (caches, adapters, cross-contract aggregators) - /// to provide an implementation that the target doesn't need to advertise, - /// while still allowing a future target to take over the contract by - /// declaring an explicit version. - /// + /// Register a contract implementation for a specific version. An empty + /// is used as the fallback when the target + /// does not advertise a version for the contract. + /// public abstract void Register(string version, Func creator) where TContract : IContract; diff --git a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/CachingContractRegistry.cs b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/CachingContractRegistry.cs index c3b21cb976cfa3..e72eb5d4dfce6d 100644 --- a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/CachingContractRegistry.cs +++ b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/CachingContractRegistry.cs @@ -48,26 +48,22 @@ public override bool TryGetContract([NotNullWhen(true)] out TContract return true; } - Func? creator = null; - if (_tryGetContractVersion(TContract.Name, out string? version) - && _creators.TryGetValue((typeof(TContract), version), out Func? versioned)) + Func? creator; + if (_tryGetContractVersion(TContract.Name, out string? version)) { - creator = versioned; + // Target declares a version — require an implementation for it. + // Do NOT fall back to the default registration in this case: a + // missing version-specific impl is a real version-skew failure + // and silently using a default would mask it. + if (!_creators.TryGetValue((typeof(TContract), version), out creator)) + { + failureReason = $"Target supports contract '{typeof(TContract).Name}' version {version}, but no implementation is registered for that version."; + return false; + } } - else if (_creators.TryGetValue((typeof(TContract), string.Empty), out Func? fallback)) + else if (!_creators.TryGetValue((typeof(TContract), string.Empty), out creator)) { - // No target-declared version, or version present but no registered - // implementation for it. Fall back to the empty-string "default" - // registration if any caller has provided one (e.g. host-side - // helpers like the layout-set cache). - creator = fallback; - } - - if (creator is null) - { - failureReason = version is null - ? $"Target does not support contract '{typeof(TContract).Name}'." - : $"Target supports contract '{typeof(TContract).Name}' version {version}, but no implementation is registered for that version."; + failureReason = $"Target does not support contract '{typeof(TContract).Name}'."; return false; } diff --git a/src/native/managed/cdac/gen/GeneratedTypeCacheContractSource.cs b/src/native/managed/cdac/gen/GeneratedTypeCacheContractSource.cs index c378a67d931d19..15e9d1efc26470 100644 --- a/src/native/managed/cdac/gen/GeneratedTypeCacheContractSource.cs +++ b/src/native/managed/cdac/gen/GeneratedTypeCacheContractSource.cs @@ -4,27 +4,9 @@ namespace Microsoft.Diagnostics.DataContractReader.DataGenerator; /// -/// Source for the IGeneratedTypeCache contract, its -/// GeneratedTypeCache_1 implementation, and the -/// TargetExtensions.GetCachedLayoutSet helper, all emitted into each -/// consuming assembly via RegisterSourceOutput (gated by -/// CompilationProvider to avoid duplicate symbols). +/// Source for the IGeneratedTypeCache contract and its supporting +/// types, emitted into each consuming assembly. /// -/// -/// The contract caches the result of LayoutSet.Resolve(target, _typeNames) -/// keyed by reference identity on the _typeNames array. The generator -/// emits exactly one static _typeNames per IData class, so reference -/// equality is the correct comparer and yields a 100% hit rate after the -/// first instantiation of each data class. -/// -/// Registration happens lazily inside GetCachedLayoutSet: the first -/// call on a given registers the -/// implementation; subsequent calls hit the cached contract instance. -/// Re-registering with the same lambda matches the no-op semantics of -/// 's last-write-wins -/// behavior, so concurrent lazy registrations from multiple call sites are -/// safe. -/// internal static class GeneratedTypeCacheContractSource { public const string HintName = "GeneratedTypeCacheContract.g.cs"; @@ -45,14 +27,8 @@ internal static class GeneratedTypeCacheContractSource namespace Microsoft.Diagnostics.DataContractReader.Generated; /// -/// Target-agnostic contract that caches per-target results of -/// , keyed by reference -/// identity on the generator-emitted static _typeNames array of each -/// IData class. Has no target-side data and is not advertised by the target's -/// contract descriptor; registered under -/// with the empty-string -/// "default" version, which the registry falls back to when no target-declared -/// version exists. +/// Target-agnostic contract that caches results +/// per target, keyed by the generator-emitted _typeNames array reference. /// internal interface IGeneratedTypeCache : IContract { @@ -61,11 +37,6 @@ internal interface IGeneratedTypeCache : IContract LayoutSet GetOrAddLayoutSet(string[] typeNames); } -/// -/// Marker type whose nameof supplies the contract name. Target-agnostic -/// contracts are not advertised in the target descriptor so this value is -/// purely diagnostic. -/// internal static class GeneratedTypeCache { } internal sealed class GeneratedTypeCache_1 : IGeneratedTypeCache @@ -88,30 +59,18 @@ public LayoutSet GetOrAddLayoutSet(string[] typeNames) public void Flush(FlushScope scope) { - // LayoutSets resolve type info from contract descriptors and managed - // metadata, both stable for the lifetime of a target snapshot and - // immutable across FlushScope.ForwardExecution. Only clear on - // FlushScope.All (matches a target re-attach / arbitrary snapshot). + // LayoutSets are immutable across ForwardExecution; only clear on All. if (scope == FlushScope.All) _cache.Clear(); } } -/// -/// Extension helpers for code generated by the cdac source generator. Bridges -/// the generated IData constructors to the -/// contract, performing first-use lazy registration so no global registration -/// step is required. -/// internal static class TargetExtensions { public static LayoutSet GetCachedLayoutSet(this Target target, string[] typeNames) { if (!target.Contracts.TryGetContract(out IGeneratedTypeCache contract)) { - // Register under the empty-string "default" version. The target - // descriptor does not advertise IGeneratedTypeCache (it has no - // target-side data), so the registry falls back to this entry. target.Contracts.Register(string.Empty, static t => new GeneratedTypeCache_1(t)); contract = target.Contracts.GetContract(); } diff --git a/src/native/managed/cdac/tests/TestInfrastructure/TestPlaceholderTarget.cs b/src/native/managed/cdac/tests/TestInfrastructure/TestPlaceholderTarget.cs index c427c1bd194276..bf9db469f9d3e1 100644 --- a/src/native/managed/cdac/tests/TestInfrastructure/TestPlaceholderTarget.cs +++ b/src/native/managed/cdac/tests/TestInfrastructure/TestPlaceholderTarget.cs @@ -635,24 +635,18 @@ public override bool TryGetContract([NotNullWhen(true)] out TContract } else if (_versions.TryGetValue(typeof(TContract), out string? version)) { - if (_creators.TryGetValue((typeof(TContract), version), out var creator)) - { - resolved = creator(_target); - } - else if (_creators.TryGetValue((typeof(TContract), string.Empty), out var fallback)) - { - resolved = fallback(_target); - } - else + // Target declares a version — require an implementation for it. + // No fallback to the empty-string default in this case. + if (!_creators.TryGetValue((typeof(TContract), version), out var creator)) { failureReason = $"Target supports contract '{typeof(TContract).Name}' version {version}, but no implementation is registered for that version."; return false; } + resolved = creator(_target); } else if (_creators.TryGetValue((typeof(TContract), string.Empty), out var fallback)) { - // No target-declared version — fall back to the empty-string - // "default" registration. Matches CachingContractRegistry. + // No target-declared version — fall back to the empty-string default. resolved = fallback(_target); } else