DeepCopy icon indicating copy to clipboard operation
DeepCopy copied to clipboard

Found a couple of issues ...

Open deipax opened this issue 7 years ago • 2 comments

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 ...

  1. 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.

deipax avatar Jan 01 '18 13:01 deipax

Thank you, @deipax! Apologies for taking a long time to respond - I was on holiday and then getting back up to speed with Orleans work. I'll try to fix those issues soon

ReubenBond avatar Jan 11 '18 01:01 ReubenBond

Oh, no worries ... I just thought you may find something helpful in the observations. What you do with them, if anything, is up to you. :)

deipax avatar Jan 19 '18 15:01 deipax