-
Notifications
You must be signed in to change notification settings - Fork 36
Description
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:
- 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 ...
- 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.