Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Shaddowed properties are not cloned properly #25

Closed
MarcinJuraszek opened this issue Oct 24, 2017 · 1 comment
Closed

Shaddowed properties are not cloned properly #25

MarcinJuraszek opened this issue Oct 24, 2017 · 1 comment

Comments

@MarcinJuraszek
Copy link
Owner

The easiest way to show what's happening is with code:

class BaseClass
{
    public int Property { get; set; }
}

class DerivedClass : BaseClass
{
    public new int Property { get; set; }
}

Following test will fail, even though it should pass

[TestMethod]
public void GetClone_DerivedTypeWithShadowedProperty_ClonnedProperly()
{
    DerivedClass source = new DerivedClass() { Property = 1 };
    ((BaseClass)source).Property = 2;

    var target = CloneFactory.GetClone(source);

    Assert.AreEqual(1, target.Property);
    Assert.AreEqual(2, ((BaseClass)target).Property);
}

Second assert fails with Assert.AreEqual failed. Expected:<2>. Actual:<0>. error.
@MarcinJuraszek MarcinJuraszek mentioned this issue Oct 24, 2017
Merged
deipax added a commit to deipax/CloneExtensions that referenced this issue Oct 24, 2017
@deipax
Potential fix for issue MarcinJuraszek#25
ed144b5
MarcinJuraszek pushed a commit that referenced this issue Oct 25, 2017
@deipax @MarcinJuraszek
Potential fix for Issue #25 (#26)
4ce9574 
* Add ArrayPrimitiveTypeExpressionFactory.
When cloning Primitives all that needs to be done is copy the source
array to the target array.  Instead of calling the
PrimitiveTypeExpressionFactory call for every element you can use the
static Array.Copy method to assist.

Depending upon the primitive type, the performance gain is different.
In the tests I have, which are coded against 4.6.1, will have a
performance gain of any where from about 98% to almost 225%.  Byte
Array, for whatever reason, has a performance gain of about 6800% ... I
have no idea why byte arrays are so much different.

Int Arrays go from about 2,386,634 ops per sec to 4,727,659 (98% increase)
String arrays go from about 1,926,782 ops per sec to 5,730,086 (197% increase)
TimeSpan arrays go from about 1,988,071 ops per sec to 4,650,697 (133% increase)
DateTime arrays go from about 1,930,501 ops per sec to 4,210,105 (118% increase)
Delegate arrays go from about 1,659,751 ops per sec to 5,404,864 (225% increase)
Byte arrays go from about 58,083 ops per sec to 4,053,648 (6879% increase)

I would like supply a unit test to easily display the difference in
performance in the master project, but given the interface, it is not
exactly straight forward.  I generated the numbers above from running
the changes suggested from a local release build in my current test
setup.

* Small fix.

* Add support for polymorphism.

In order to support polymorphism, we have to find the runtime type and run the appropriate CloneDelegate against the source object.  I added the CloneIdDelegateCache to assist with this process.  I needed to make a couple changes to the ComplexTypeExpression factory to add polymorphism support.  In order to maintain backwards compatibility, if an initializer was supplied by the user, that should be used before the call to the new code.  I added a new test class that asserts several scenarios, including the scenario outlined in issue #7.  Also in the new unit test, I added some SpeedComparions tests  (which I commented out for integration purposes) to help give an idea of what type of performance hit this change will cause.

Let me know what you think.

* Revert "Add support for polymorphism."

This reverts commit d2035d9.

* Supply a Expression factory for a List of primitives.

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 factorys do not check flags at all.

* There is no need to clone any primitive types, they can be used directly.  This change set removes all cloning calls for primitives.  Here is a incomplete summary of the performance changes (all figures are in operations per second).

Clone an Array of classes: 18,761 ->26,702 (about 42% increase)
Clone an Array of Structs: 27,461 ->40,526 (about 47% increase)
Clone a simple class (all primitive properties): 1,973,684 -> 2,918,287 (about 47% increase)
Clone a complex class: 1,776,198 -> 2,056,202 (about 15% increase)
Clone a Class:List<Class>: 0.999 -> 1.176 (about 17% increase)
Clone a Class:List<int>: 1.608 -> 3.115 (about 93% increase)
Clone a KeyValue<int,int>: 9,569,377 -> 14,903,129 (about 55% increase)
Clone a Tuple<int,int>: 3,992,015 -> 10,033,444 (about 151% increase)
Clone a Nullable<int>: 19,607,843 -> 33,025,099 (about a 68% increase)

* I am not sure if this is considered a bug or not, but Tuples are classes.  I added Check for nulls.  Add them to the cloned object dictionary and reuse them if found.  Added unit tests to verify this functionality.

* Leverage backing fields for automatic properties.

* Revert "Leverage backing fields for automatic properties."

This reverts commit af1740c.

* Potential fix for issue #25

* Add Unit tests for Abstract and Virtual properties.

* Add a "new" Field to unit test base class and sub class.
@MarcinJuraszek
Copy link
Owner Author

Fixed by #26 (review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@MarcinJuraszek