search

MarcinJuraszek / CloneExtensions

Clone extension method library. Performs fast, deep or shallow clone using simple assignment operations generated by Expression Tree runtime code compilation.
Apache License 2.0
115 stars 30 forks source link

Supply a Expression factory for a List of primitives. #18

Closed deipax closed 7 years ago

deipax commented 7 years ago

Similar to the Array of primitives, there is no need to call the clone method for primitivies. Since List is so common, providing a custom Expression factory should be quite useful. According to my testing, here are the performance differences:

List of Strings: 22,815 ops per second ->1,111,000 (about 4700% increase) List of Bytes: 46,153 ops per second ->1,199,880 (about 2500% increase) List of Ints: 44,863 ops per second -> 990,000 (about 2100% increase) List of Delegates: 1,119,194 ops per second ->5,221,409 (about 365% increase) List of DateTime: 1,274,697 ops per second -> 4,629,166 (about 263% increase) List of TimeSpan: 1,251,564 ops per second -> 5,221,409 (about 317% increase)

One thing I had to do in order to address an assertion made by the unit tests was to not clone the element is the Flag.CollectionItems was missing. Which I found surpising, should there be such an assertion of Arrays of items as well? Currently the Array expression factories do not check flags at all.

MarcinJuraszek commented 7 years ago

Interesting. If we don't clone the array elements when Flag.CollectionItems flag is missing that Array will always end up being an empty array. I'm not what the right behavior here is.

On the other hand, defaultFlags is set to CloningFlags.Fields | CloningFlags.Properties | CloningFlags.CollectionItems so by default (when no flags are provided) we would still copy the array following more intuitive experience, so maybe changing the behavior isn't that bad.

What do you think should happen when somebody explicitly passes CloningFlags.Fields | CloningFlags.Properties while cloning an array (or an object with an array-typed field/property)?

deipax commented 7 years ago

I would avoid making breaking changes at this time.

Any user that depends upon the current implementation can use the NonCloneAttribute in order to ignore arrays they do not need, at least on classes that they author. For classes they do not author, there is no current way to ignore those arrays ... which they have accepted or do not use the tool at all. If I were a user and I was using this code in production, such a change would be upsetting.

If a breaking change should be introduced, it would be a good time to also consider different configuration options for cloning classes the user authors and also for classes they do not, it should be probably be in a dev branch where the users can work according to their risk tolerance.

Just my .02.

MarcinJuraszek commented 7 years ago

I agree that breaking changes are not ideal, but sometimes they can't be avoided and benefit of better, more natural behavior outweigh the pain of upgrading. For now let's not do it, but if there is more places where breaking with current behavior would be beneficial for the quality and performance of the library we can bundle them all together to a single release (probably 2.0 at that point).

I'll merge this PR as is for now.

Thanks again for your contributions!

deipax commented 7 years ago

My pleasure. I have a few more performance changes to offer, I will continue to submit them when I have time.