reduce lookups per type#32
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on April 2. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
There was a problem hiding this comment.
Pull request overview
This PR reduces repeated reflection/cache lookups during cloning by introducing a consolidated per-type “shape” cache and by emitting a specialized clone entrypoint for sealed reference types (so the runtime type lookup can be skipped when the compile-time type is exact).
Changes:
- Add a cached
TypeShape(members, ignored-event info, cycle field types, and derived flags) and update call sites to use it. - Introduce
CloneClassInternalExact<T>and emit it for sealed member types to avoid per-call runtime type metadata lookup. - Add method-info caching for the new exact sealed-type clone method.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/FastCloner/Code/StaticMethodInfos.cs | Adds cached MethodInfo + generic-method cache for CloneClassInternalExact<T>. |
| src/FastCloner/Code/FastClonerGenerator.cs | Switches multiple metadata checks to TypeShape and adds CloneClassInternalExact<T>. |
| src/FastCloner/Code/FastClonerExprGenerator.cs | Adds TypeShape retrieval/building and emits exact clone calls for sealed member types. |
| src/FastCloner/Code/FastClonerCache.cs | Introduces TypeShape and a dedicated cache for it; removes older separate caches. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Deep Clone Benchmarks
Current FastCloner vs DeepCloner
FastCloner vs latest
|
| Status | Benchmark | Delta Time | Delta Alloc |
|---|---|---|---|
| 🟢 | DynamicWithArray | -15% faster | ~same |
| ⚪ | DynamicWithDictionary | +5% slower | ~same |
| ⚪ | DynamicWithNestedObject | ~same | ~same |
| ⚪ | FileSpec | -4% faster | ~same |
| 🟢 | LargeEventDocument_10MB | -7% faster | ~same |
| ⚪ | LargeLogBatch_10MB | ~same | ~same |
| ⚪ | MediumNestedObject | ~same | ~same |
| ⚪ | ObjectDictionary_50 | ~same | ~same |
| ⚪ | ObjectList_100 | -2% faster | ~same |
| 🟢 | SmallObject | -6% faster | ~same |
| ⚪ | SmallObjectWithCollections | -2% faster | ~same |
| ⚪ | StringArray_1000 | ~same | ~same |
Regressions
- none
Improvements
DynamicWithArray: time -15% faster, alloc ~sameLargeEventDocument_10MB: time -7% faster, alloc ~sameSmallObject: time -6% faster, alloc ~same
Mixed changes
- none
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/FastCloner/Code/FastClonerExprGenerator.cs:33
GetClassCloneMethodnow acceptsmemberTypebut does not use it, and the newly-addedCloneClassInternalExact/MakeExactClassCloneMethodInfopath appears unused. If the goal is to reduce per-member runtime type/metadata lookups, consider selecting the exact generic clone method whenmemberTypeis sealed (and not shallow/no-tracking); otherwise remove the parameter/dead code.
private static MethodInfo GetClassCloneMethod(Type memberType, bool useShallowClassClone, bool skipCycleTracking)
{
if (useShallowClassClone)
return StaticMethodInfos.DeepClonerGeneratorMethods.CloneClassShallowAndTrack;
💡 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 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/FastCloner/Code/FastClonerExprGenerator.cs:37
GetClassCloneMethodnow takesmemberType, but it’s not used in the method body. As a result, the newCloneClassInternalExact/MakeExactClassCloneMethodInfopath is never selected anywhere in the codebase (verified by searching forMakeExactClassCloneMethodInfousages). Either wirememberTypein here (e.g., use the exact generic cloner for sealed member types to reduce runtime type/metadata lookups) or drop the parameter/new cache to avoid dead code and confusion.
private static MethodInfo GetClassCloneMethod(Type memberType, bool useShallowClassClone, bool skipCycleTracking)
{
if (useShallowClassClone)
return StaticMethodInfos.DeepClonerGeneratorMethods.CloneClassShallowAndTrack;
return skipCycleTracking
? StaticMethodInfos.DeepClonerGeneratorMethods.CloneClassInternalNoTracking
: StaticMethodInfos.DeepClonerGeneratorMethods.CloneClassInternal;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ReSharper disable once CheckNamespace | ||
| namespace System.Runtime.CompilerServices | ||
| { | ||
| internal static class IsExternalInit; |
There was a problem hiding this comment.
IsExternalInit is declared with a trailing semicolon and no body, which is not valid C# syntax. This will break compilation for the non-MODERN TFMs (net46/netstandard2.0) that need the init-only setter polyfill. Define it with an empty body instead (the usual internal static class IsExternalInit { }).
| internal static class IsExternalInit; | |
| internal static class IsExternalInit | |
| { | |
| } |
| ClonerCache<T>.CacheEntry cacheEntry = ClonerCache<T>.GetCurrent(); | ||
| if (!FastClonerCache.HasActiveTypeBehaviorOverrides && cacheEntry.IsSafe) | ||
| return obj; | ||
|
|
||
| Type objType = typeof(T); | ||
| FastClonerCache.TypeCloneMetadata metadata = cacheEntry.Metadata ?? GetTypeMetadata(objType, state); | ||
|
|
||
| // Keep exact and polymorphic class cloning on the same decision path. | ||
| return (T?)CloneClassInternalTyped(obj, objType, state, metadata); |
There was a problem hiding this comment.
CloneClassInternalExact<T> uses typeof(T) for metadata/cloner selection, which will produce incorrect results if it’s ever called with a derived runtime instance (e.g., T is a base type but obj is a subclass): derived fields/overrides and per-type behaviors would be skipped. Consider enforcing the contract (e.g., check obj.GetType() == typeof(T) and fall back to CloneClassInternal/polymorphic path if not), or otherwise ensure it’s only callable for sealed/exact types (at least via a debug assert).
| ClonerCache<T>.CacheEntry cacheEntry = ClonerCache<T>.GetCurrent(); | |
| if (!FastClonerCache.HasActiveTypeBehaviorOverrides && cacheEntry.IsSafe) | |
| return obj; | |
| Type objType = typeof(T); | |
| FastClonerCache.TypeCloneMetadata metadata = cacheEntry.Metadata ?? GetTypeMetadata(objType, state); | |
| // Keep exact and polymorphic class cloning on the same decision path. | |
| return (T?)CloneClassInternalTyped(obj, objType, state, metadata); | |
| Type runtimeType = obj.GetType(); | |
| Type declaredType = typeof(T); | |
| // If the runtime type is not exactly T, fall back to the polymorphic path | |
| // to ensure derived fields/overrides and per-type behaviors are respected. | |
| if (runtimeType != declaredType) | |
| return (T?)CloneClassInternal(obj, state); | |
| ClonerCache<T>.CacheEntry cacheEntry = ClonerCache<T>.GetCurrent(); | |
| if (!FastClonerCache.HasActiveTypeBehaviorOverrides && cacheEntry.IsSafe) | |
| return obj; | |
| FastClonerCache.TypeCloneMetadata metadata = cacheEntry.Metadata ?? GetTypeMetadata(declaredType, state); | |
| // Keep exact and polymorphic class cloning on the same decision path. | |
| return (T?)CloneClassInternalTyped(obj, declaredType, state, metadata); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
src/FastCloner/Code/FastClonerGenerator.cs:236
BuildTypeMetadatacomputeshasBehaviorSensitiveMembers, but it’s not considered when selectingCyclePolicy. UsingWorklistfor types with ignored members can be incorrect because the worklist path starts with a shallow MemberwiseClone and the patching step (ClonerToExprGenerator) does not currently apply member-level Ignore semantics (defaults), leaving ignored fields copied from the source. Consider falling back toTrackReferenceswhenhasBehaviorSensitiveMembersis true, or ensure the worklist patching step explicitly resets ignored members to default.
FastClonerCache.CyclePolicy cyclePolicy = !canHaveCycles
? FastClonerCache.CyclePolicy.None
: hasDirectSelfReference
? FastClonerCache.CyclePolicy.Worklist
: FastClonerCache.CyclePolicy.TrackReferences;
src/FastCloner/Code/FastClonerExprGenerator.cs:37
GetClassCloneMethodnow takes amemberTypeparameter but doesn’t use it. This adds noise and suggests there’s member-type-specific behavior that doesn’t exist; either remove the parameter or use it (if intended) to avoid confusion for future maintainers.
private static MethodInfo GetClassCloneMethod(Type memberType, bool useShallowClassClone, bool skipCycleTracking)
{
if (useShallowClassClone)
return StaticMethodInfos.DeepClonerGeneratorMethods.CloneClassShallowAndTrack;
return skipCycleTracking
? StaticMethodInfos.DeepClonerGeneratorMethods.CloneClassInternalNoTracking
: StaticMethodInfos.DeepClonerGeneratorMethods.CloneClassInternal;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Performs deep (full) copy of object and related graph | ||
| /// </summary> | ||
| public T DeepClone() => FastClonerGenerator.CloneObject(obj); | ||
| public T DeepClone() => FastClonerGenerator.CloneObject(obj)!; |
There was a problem hiding this comment.
DeepClone() returns non-null T, but FastClonerGenerator.CloneObject(obj) is nullable (T?) and returns default when the receiver is null. The added null-forgiving operator hides that and makes the nullability contract incorrect for callers. Consider changing the extension method signature to return T? (or explicitly throw on null) instead of using !.
| public T DeepClone() => FastClonerGenerator.CloneObject(obj)!; | |
| public T? DeepClone() => FastClonerGenerator.CloneObject(obj); |
| public TTo DeepCloneTo<TTo>(TTo objTo) where TTo : class, T => (TTo)FastClonerGenerator.CloneObjectTo(obj, objTo, true)!; | ||
|
|
||
| /// <summary> | ||
| /// Performs shallow copy of object to existing object | ||
| /// </summary> | ||
| /// <returns>existing filled object</returns> | ||
| /// <remarks>Method is valid only for classes, classes should be descendants in reality, not in declaration</remarks> | ||
| public TTo ShallowCloneTo<TTo>(TTo objTo) where TTo : class, T => (TTo)FastClonerGenerator.CloneObjectTo(obj, objTo, false); | ||
| public TTo ShallowCloneTo<TTo>(TTo objTo) where TTo : class, T => (TTo)FastClonerGenerator.CloneObjectTo(obj, objTo, false)!; |
There was a problem hiding this comment.
DeepCloneTo/ShallowCloneTo cast the result to TTo and apply !, but CloneObjectTo(...) can return null (e.g., when objTo is null) and these methods are callable with a null receiver. Either adjust the signatures to return TTo? and remove the null-forgiving operator, or enforce non-null inputs (throw) so the non-null return type is truthful.
| /// Performs shallow (only new object returned, without cloning of dependencies) copy of object | ||
| /// </summary> | ||
| public T ShallowClone() => ShallowClonerGenerator.CloneObject(obj); | ||
| public T ShallowClone() => ShallowClonerGenerator.CloneObject(obj)!; |
There was a problem hiding this comment.
ShallowClone() returns non-null T, but ShallowClonerGenerator.CloneObject(obj) returns T? and returns default when the receiver is null. The ! masks that and produces an incorrect nullability contract; prefer returning T? (or throwing on null input) instead of using the null-forgiving operator.
| public T ShallowClone() => ShallowClonerGenerator.CloneObject(obj)!; | |
| public T? ShallowClone() => ShallowClonerGenerator.CloneObject(obj); |
| /// Internal helper class used to perform shallow object cloning | ||
| /// </summary> | ||
| public abstract class ShallowObjectCloner | ||
| internal static class ShallowObjectCloner |
There was a problem hiding this comment.
ShallowObjectCloner was changed from public to internal, which is a breaking change for any external consumers referencing this type (even if it’s “not intended” public API). If this needs to be internal, consider keeping a public shim/obsolete type for compatibility, or confirm this is acceptable as a major-version breaking change.
| internal static class ShallowObjectCloner | |
| public static class ShallowObjectCloner |
No description provided.