2

Is there a faster way to clone the items than what is shown below?

private List<T> CloneItems(List<T> itemsToClone) {
    lock (dataLocker) {
        Stopwatch sw = Stopwatch.StartNew();

        int numItems = itemsToClone.Count;
        List<T> itemsToBeReturned = new List<T>(numItems);

        for (int i = 0; i < numItems; i++) {
            itemsToBeReturned.Add((T)itemsToClone[i].Clone());
        }

        Debug.WriteLine("CloneItems(ms): " + sw.Elapsed.TotalMilliseconds.ToString("F3"));
        return itemsToBeReturned;
    }
}

EDIT: I need a deep copy and am currently using it with the following object:

public class TimestampedDouble {
    public long Timestamp { get; set; }
    public double Voltage { get; set; }
    public double Current { get; set; }
    ...
    public override object Clone() {
        return MemberwiseClone();
    }
}
nb1forxp
  • 385
  • 2
  • 14
  • 4
    Shallow clone or deep clone? – Ron Beyer Apr 23 '15 at 19:18
  • Why the downvotes? What did I do wrong? – nb1forxp Apr 23 '15 at 19:43
  • @RonBeyer If those three properties are the only ones, then in *this* specific case, it would be a deep copy since the properties are all value types. – Cameron Apr 23 '15 at 20:01
  • How many of these `TimestampedDouble` are you copying? Have you tried something like [this answer](http://stackoverflow.com/a/129395/2607840)? – Cameron Apr 23 '15 at 20:02
  • @Cameron I'm copying hundreds of TimestampedDoubles every 20ms. I looked at your linked answer and tried this variant (which is supposedly 3X faster than the serialization approach) and it is ~20X slower than my method in the original post: https://raw.github.com/Burtsev-Alexey/net-object-deep-copy/master/ObjectExtensions.cs – nb1forxp Apr 23 '15 at 20:17
  • @Cameron, it wasn't apparent when I posted that, the data structure wasn't in the original post. – Ron Beyer Apr 23 '15 at 20:23
  • As a test, try doing it without the generics, I'm guessing boxing/unboxing has a negative effect on run times. Also be aware that .NET and Windows aren't real-time systems, so you are hoping for 20ms, in reality it could be much bigger. You'll have to set a higher priority thread to get a little faster response times, but its no guarantee. – Ron Beyer Apr 23 '15 at 20:27
  • `MemberwiseClone` is a [_shallow copy_](https://msdn.microsoft.com/en-us/library/system.object.memberwiseclone%28v=vs.110%29.aspx) – Zer0 Apr 23 '15 at 22:57

3 Answers3

1

Like others have mentioned, you need to find the actual bottleneck. Your code above is fairly tight in terms of what it does. Likely, it's the clone and not your loop. However, it turns out that Microsoft already did that loop for you in the ConvertAll method and you could reimplement your code above like this:

var finallist = itemsToClone.ConvertAll<T>(o => (T)o.Clone());

However, I'm going to guess that it won't be a win since if I had been asked to implement ConvertAll, I would have done something close to your loop. And in fact, that's pretty much what MS did.

And that would point right back to your Clone method as the culprit.

plinth
  • 48,267
  • 11
  • 78
  • 120
0

You can use an Extension method like this:

static class Extensions
{
    public static IList<T> Clone<T>(this IList<T> listToClone) where T: ICloneable
    {
        return listToClone.Select(item => (T)item.Clone()).ToList();
    }
}
Aram
  • 5,537
  • 2
  • 30
  • 41
  • 1
    Given that the bulk of the time in this operation is likely to be spent in the `Clone` method, it is unlikely that this will have a significant effect on the amount of time taken by the total operation. But, since I could be wrong, you could include the reasoning behind why you think this is faster. – Steve Mitcham Apr 23 '15 at 19:22
  • FYI, Select() gives an IEnumerable. ToList() won't know how many items need to be allocated a priori, so it will have to grow the list (depending on the number of items). – plinth Apr 23 '15 at 19:54
  • @Aram I tried this variant (not as an extension method) and it's about 2X slower than my method shown in the question: List listCloned = itemsToClone.Select(i => (T)i.Clone()).ToList(); – nb1forxp Apr 23 '15 at 20:20
0

Looks pretty solid to me. Other suggestions would be less code, but not faster. Especially the suggestions when the array size is not pre-allocated.

Best idea I have is making TimestampedDouble a struct will be faster to get rid of the allocations and garbage collections.

But if you can restructure the rest of the code to not need entire lists, and pipeline instead, would be the best bet.

Nick Whaley
  • 2,729
  • 2
  • 21
  • 28