reubenbond / deepcopy Goto Github PK

        private class MyClass
        {
            
        }
        [Fact]
        public void CopyObjectHashSet()
        {
            HashSet<MyClass> original = new HashSet<MyClass>();
            original.Add(new MyClass());

            var result = DeepCopier.Copy(original);

            Assert.NotSame(original, result);
            foreach (MyClass myClass in result) {
                Assert.True(result.Contains(myClass));
            }
        }

After deep copy , then Contains method return false.

Reuben,
I found your project via Marcin Juraszek, the author of CloneExtensions. I have spent sometime reviewing your work (which I found very interesting) and updating my own project with observations I have made. During my review, I found some potential issues ... DeepCopy fails some of my integrations tests. I figured I would offer to you what I have found.

First, if you would like to look at my source, you may find it here: https://github.com/deipax/Deipax

I placed all potential issues in a single unit test file, which you can find here: https://github.com/deipax/Deipax/blob/master/Source/UnitTests.Cloning/DeepCopyIssues.cs

I have a series of benchmarks, generated by BenchmarkDotNet, which I ran against DeepCopy and my source. The results can be found here if you are interested in looking at performance comparisons:

https://github.com/deipax/Deipax/tree/master/Source/Benchmarks.Cloning/BenchmarkDotNet.Artifacts/results/DeepCopy
https://github.com/deipax/Deipax/tree/master/Source/Benchmarks.Cloning/BenchmarkDotNet.Artifacts/results/Deipax

My review of the benchmarks led to the following observations:

1. In ArrayCopier.cs, in the CopyArray method, there is this code:

if (DeepCopier.CopyPolicy.IsImmutable(elementType))
{
     Array.Copy(originalArray, copyArray, originalArray.Length);
    // Return copyArray here?
}

Im guessing, that the copyArray should be returned but it is not ...

2. In the CopierGenerator, you cache delegates for run time types in a ConcurrentDictionary. Take a look at:
   https://github.com/deipax/Deipax/blob/master/Source/Deipax.Cloning/Common/Cloner.cs
   https://github.com/deipax/Deipax/blob/master/Source/Deipax.Core/Common/QuickCache.cs

The storage/retrieval of delegates takes about 1/2 the time.

That is about it, if you have any questions or comments feel free to fire away.

Thanks,
-Jeff

Edit updated the benchmark links.

I have a series of tests that I run and build against net6.0, net5.0 and netcoreapp3.1 but DeepCopy is only failing on net6.0

This issue seems to be around trying to copy an IEnumerable or an object that contains an IEnumerable.

Message: 
System.InvalidProgramException : Common Language Runtime detected an invalid program.

Stack Trace: 
EnumeratorDeepCopier(Enumerator , CopyContext )
SelectListIterator2DeepCopier(IEnumerable1 , CopyContext )
CopierGenerator`1.Copy(T original, CopyContext context)
DeepCopier.Copy[T](T original)
Tokenizer.TokenizeAsync[T](T objectWithSensitiveData)
TokenizerTests.Tokenize_IEnumberableObject() line 151
--- End of stack trace from previous location ---

Your article is nice and well written :)

As you asked for pull requests, I'll have one ready in a few minutes for more immutable support (e.g. keyvaluepair, url, version, tuples)

A few Comments and questions:

• Your type keys
  

  DeepCopy/src/DeepCopy/DeepCopier.cs

  Lines 51 to 60 in 8ddf2b6

  	var type = original.GetType();
  	if (!type.IsValueType)
  	{
  	// Handle arrays specially.
  	var originalArray = original as Array;
  	if (originalArray != null) return (T)CopyArray(originalArray, context);
  	}
  	var typedCopier = CopierGenerator.GetOrCreateCopier<T>(type);
  	return typedCopier(original, context);

  
  This is rather strange, you have a generic method call for T but still pass the type in. Actually you then use both types as key in
  

  DeepCopy/src/DeepCopy/CopierGenerator.cs

  Lines 36 to 38 in 8ddf2b6

  	var parameterType = typeof(T);
  	var key = (type, parameterType);
  	if (!this.copiers.TryGetValue(key, out var untypedCopier))

  
  I think one type is enough there, either you have the typed method or you pass the type as parameter.

• You need to cache the immutable delegate for immutables too:
  

  DeepCopy/src/DeepCopy/CopierGenerator.cs

  Lines 28 to 34 in 8ddf2b6

  	public DeepCopyDelegate<T> GetOrCreateCopier<T>(Type type)
  	{
  	if (this.copyPolicy.IsImmutable(type))
  	{
  	T ImmutableCopier(T original, CopyContext context) => original;
  	return ImmutableCopier;
  	}

  
  You create a new delegate and check the rather cost expensive type check for immutability each and every call

• You should use GetOrAdd instead of your own TryGetValue logic here:
  

  DeepCopy/src/DeepCopy/CopierGenerator.cs

  Lines 38 to 42 in 8ddf2b6

  	if (!this.copiers.TryGetValue(key, out var untypedCopier))
  	{
  	untypedCopier = this.CreateCopier<T>(type);
  	this.copiers.TryAdd(key, untypedCopier);
  	}

I'm not certain that your CachedReadConcurrentDictionary is actually threadsafe regarding the optimizations there, I know it's used in Orleans too, you even added a few MemoryBarriers, which are not in the original source, but they should not affect the items below.
Anyway my point:

• Thread 1 does a TryAdd, the call succeeds, so it's after line
  

  DeepCopy/src/DeepCopy/CachedReadConcurrentDictionary.cs

  Line 162 in 8ddf2b6

  if (this.dictionary.TryAdd(key, value))

• Thread 2 does a lookup at this moment in time via the indexer
  

  DeepCopy/src/DeepCopy/CachedReadConcurrentDictionary.cs

  Lines 186 to 188 in 8ddf2b6

  	public TValue this[TKey key]
  	{
  	get => this.GetReadDictionary()[key];

  
  

  DeepCopy/src/DeepCopy/CachedReadConcurrentDictionary.cs

  Lines 202 to 203 in 8ddf2b6

  	[MethodImpl(MethodImplOptions.AggressiveInlining)]
  	private IDictionary<TKey, TValue> GetReadDictionary() => this.readCache ?? this.GetWithoutCache();

  
  This will return the old dictionary if the InvalidateCache method was not yet executed.

I personally recommend to remove that optimization from this project and you really should go with a very very fine comb through the original code in Orleans, this might produce subtle bugs (e.g. value not in dictionary, even though it was added or vice versa) depending on your usage pattern. Relying on an implementation detail, being that Dictionary<,> is actually thread-safe for reads is a bad idea anyway.

What I really want is something like F# records. Those a helpful feature called Copy and Update Expressions:

type Person = {
    FirstName: string
    LastName: string
    Email: string
}

let person = { FirstName = "first"; LastName = "last"; Email = "email" }
let updatedEmail = { person with Email = "newEmail" } // Copy and Update Expression

Will be nice to create extension method based on deepcopier that will copy and override values like this:

class Person
{
      public string FirstName { get; }
      public string LastName { get; }

     public Person(string firstName, string lastName)
    {
          FirstName = firstName;
          LastName = lastName;
    }
}

var person = new Person("bob", "jonhs");
var person2 = person.Update({ FirstName = "michael" });
var person3 = person.Update(p => p.FirstName, "michael");

Hi, during benchmarking of #1 I noticed, that the SimpleClass clone is quite slow, due to the recording of the same instance logic.
I benchmarked a few different versions:

• Your current one
• Removing the context handling in the IL code
• Removing the context handling and calling the delegate directly
• Expression Tree version for properties I'm currently using
• simply calling doing a new SimpleClass with the values (pure ctor and property assignment)

I think it might useful to find a way how to clone an object without the context and only handle the context if it's necessary.
For example:
Only add the context handling part for arrays, lists of classes etc (primitive types and structs shouldn't need it, you already do that most of the time afaik) and provide two versions, one for lists and one for direct calls

I admit this might get ugly with nested objects, but nested objects usually are bigger so it won't impact as much, if at all, I guess having a fast path for plain and simple c# objects would nice.

Befor the code:

var typeField = DeepCopier.FieldBuilder.GetOrCreateStaticField(type);
il.Emit(OpCodes.Ldsfld, typeField);
il.Emit(OpCodes.Call, DeepCopier.MethodInfos.GetUninitializedObject);
il.Emit(OpCodes.Castclass, type);
il.Emit(OpCodes.Stloc_0);

After the code:

il.Emit(OpCodes.Ldtoken, type);
il.Emit(OpCodes.Call, DeepCopier.MethodInfos.GetTypeFromHandle);
il.Emit(OpCodes.Call, DeepCopier.MethodInfos.GetUninitializedObject);
il.Emit(OpCodes.Castclass, type);
il.Emit(OpCodes.Stloc_0);

        [Fact]
        public void CopyHashSet()
        {
            HashSet<string> original = new HashSet<string>(StringComparer.OrdinalIgnoreCase);

            original.Add("A");
            original.Add("B");

            var result = DeepCopier.Copy(original);

            Assert.NotSame(original, result);
            Assert.True(result.Contains("b"));
            Assert.True(result.Contains("a"));

        }

System.TypeInitializationException : The type initializer for 'DeepCopy.CopierGenerator1' threw an exception. ---- System.NotSupportedException : Illegal one-byte branch at position: 10. Requested branch was: 136. at DeepCopy.CopierGenerator1.Copy(T original, CopyContext context) in G:\source\DeepCopy-master\DeepCopy-master\src\DeepCopy\CopierGenerator.cs:line 26
at DeepCopy.DeepCopier.Copy[T](T original) in G:\source\DeepCopy-master\DeepCopy-master\src\DeepCopy\DeepCopier.cs:line 26
at DeepCopy.UnitTests.CopyTests.CopyHashSet() in G:\source\DeepCopy-master\DeepCopy-master\test\DeepCopy.UnitTests\CopyTests.cs:line 170
----- Inner Stack Trace -----
at System.Reflection.Emit.ILGenerator.BakeByteArray()
at System.Reflection.Emit.DynamicResolver..ctor(DynamicILGenerator ilGenerator)
at System.Reflection.Emit.DynamicILGenerator.GetCallableMethod(RuntimeModule module, DynamicMethod dm)
at System.Reflection.Emit.DynamicMethod.GetMethodDescriptor()
at System.Reflection.Emit.DynamicMethod.CreateDelegate(Type delegateType)
at DeepCopy.CopierGenerator1.CreateCopier(Type type) in G:\source\DeepCopy-master\DeepCopy-master\src\DeepCopy\CopierGenerator.cs:line 150 at DeepCopy.CopierGenerator1..cctor() in G:\source\DeepCopy-master\DeepCopy-master\src\DeepCopy\CopierGenerator.cs:line 17

Demo Code:

            var originalArray = new string[] { "123", "hello!" };
            var resultArray = DeepCopier.Copy((object)original);
            Assert.Equal(originalArray, resultArray);
            Assert.NotSame(originalArray, resultArray);

Stace Trace:

System.ArgumentException : Cannot bind to the target method because its signature or security transparency is not compatible with that of the delegate type.
at System.Reflection.RuntimeMethodInfo.CreateDelegateInternal(Type delegateType, Object firstArgument, DelegateBindingFlags bindingFlags, StackCrawlMark& stackMark)
at System.Reflection.RuntimeMethodInfo.CreateDelegate(Type delegateType)
at DeepCopy.CopierGenerator1.CreateArrayCopier(Type type) at DeepCopy.CopierGenerator1.CreateCopier(Type type)
at System.Collections.Concurrent.ConcurrentDictionary2.GetOrAdd(TKey key, Func2 valueFactory)
at DeepCopy.CopierGenerator`1.Copy(T original, CopyContext context)
at DeepCopy.DeepCopier.Copy[T](T original)
at DeepCopy.UnitTests.CopyTests.CanCopyArrays()

        private class MyClass
        {
            public readonly ICollection<string> Collection = new List<string>();
        }

        [Fact]
        public void CanCopyInterfaceField()
        {
            MyClass original = new MyClass();

            original.Collection.Add("A");

            var result = DeepCopier.Copy(original);

            Assert.NotSame(original, result); //Failure
            Assert.NotSame(original.Collection, result.Collection); //Failure
        }

The result is failure.

There is a bug in the immutability check, I had the pull request for. Currently we only check if the genericTypeDef is immutable, but we need to check each TypeArg of the constructed type too.
Currently:
KeyValuePair<string,string> -> Immutable
KeyValuePair<string,Poco> -> Immutable
Expected:
KeyValuePair<string,Poco> -> Mutable

Repro:

[Fact]
public void CanCopyMutableKeyValuePair()
{
    var original = new KeyValuePair<string, Poco>("Hello", new Poco());
    var result = DeepCopier.Copy(original);
    Assert.Same(original.Key, result.Key);
    Assert.NotSame(original.Value, result.Value);
}

I tried to fix that by changing the check to:

 if (type.IsGenericType && this.policies.TryGetValue(type.GetGenericTypeDefinition(), out result))
 {
     if (result == Policy.Immutable)
     {
         var generigArgs = type.GetGenericArguments();
         result = generigArgs.All(IsImmutable) ? Policy.Immutable : Policy.Mutable;
     }
     return this.policies[type] = result;
 }

but then I get ExecutionEngine Exceptions after running tests and not necessarily in the same order.