0

i have been tasked with updating some legacy code and really need to decrease the run time on part of it. The below list is loaded quite often. I have managed to reduce the time from about a minute to around fifteen seconds, but it really needs to go down further. The following lines of code work fairly well, but I am trying to squeeze everything I can out of it.

List<MyObject> _moList = new List<MyObject>(DB.GetAll(queryString, parameters, MyObject.Extract));
_moList.AsParallel().ForAll(s => s.RelativeCost = GetRelativeCost(s));

So I have a few questions. First, is it possible to combine those two lines into one line, and if so, how? Second, would doing this improve the performance at all?

Some items of not are that MyObject has around forty properties (not sure if that is pertinent or not) and GetRelativeCost is somewhat expensive when it comes to time/cpu cycles (hence the parallel run).

Any help would be greatly appreciated!

P.S. I am working other angles too, in particular trying to reduce the "cost" of GetRelativeCost, but I need to get every cycle I can get out of this to make it acceptable to the user.

Opus Krokus
  • 241
  • 1
  • 2
  • 3
  • 1
    it looks like you're going to the database...have you profiled the query to see if that can be sped up? – kzhen Mar 11 '13 at 21:25
  • Are you looking to improve average performance (over antire lifetime of application) or worst-case prformance on each run? If the former, you could try caching the result each time and testing to see if the source data changed before reloading. – Pieter Geerkens Mar 11 '13 at 21:26
  • Why are you using `AsParallel` rather than `Parallel.ForEach`? – Zaid Masud Mar 11 '13 at 21:28
  • kzhen - yes Pieter - It is the former, I'll look to see if there is some way to do this Zaid - No reason. Is there some reason to use one over the other? – Opus Krokus Mar 12 '13 at 13:39

2 Answers2

1
  1. Have you run the profiler? Are you sure you are optimizing the bottleneck?
  2. Why are you trying to optimize list processing rather than GetRelativeCost?
  3. What are DB.GetAll? Are you sure it's working in optimal way?
  4. Do I assume right MyObject.Extract is some kind of object hydration function? Are you sure it's optimal?
  5. Why GetRelativeCost is executed on list loading? Wouldn't it make sense to delay its execution?

More on the last point. If your GetRelativeCost have some side effects - you'd better remove them anyway, since it's a violation of DRY principle. If not, you might want to rewrite RelativeCost to allow for lazy initialization, like

private int? _relativeCost
public int RelativeCost {
    get {
       if  (!_relativeCost.HasValue)
            _relativeCost = GetRelativeCost();
       return _relativeCost;
    }
}

This way you delay the GetRelativeCost execution until it really needed. this way you are optimizing by not calculating GetRelativeCost for entities that don't really need RelativeCost set, as well as speeding up that particular piece of code by delaying the costly calculations in GetRelativeCost at cost of increased calculations later on.

To sum up: run profiler, determine the bottleneck, optimize the bottleneck. If bottleneck appears to be in GetRelativeCost - try making it lazy as I described above.

J0HN
  • 26,063
  • 5
  • 54
  • 85
  • 1. Yes, several times 2. I am also trying to optimize get relative cost 3. No control over this. It is DB.GetAll is referring to a third party assembly to which I have no source code. To remove all references would require a major rewrite of the code. 4. I'm open to suggestions. See below. – Opus Krokus Mar 12 '13 at 14:24
  • 5. a - The next line is `_objectCache = new List(_moList.Select(s => new MyObjectDisplayer(s)));` I then have to access the object through another object. Moving it to the list load cut my time by about 10%. – Opus Krokus Mar 12 '13 at 14:25
  • b - MyObject is a part of a larger construct. To calculate the cost, it would have to have knowledge of that larger construct. For instance, the larger construct might be 48 inches wide. MyObject could be 8, 12, 24, or 48 inces. The 8 inch one has a higher cost than the 48 inch one because it requires that 6 be used. Would it be better to do the lazy loading by having MyObject know about its parent, or have the parent calculate the cost based on the children? – Opus Krokus Mar 12 '13 at 14:25
  • `public static IEnumerable Extract(DbDataReader dr) { int oModelNumber = dr.GetOrdinal("ModelNumber"), oIsStandardProduct = dr.GetOrdinal("IsStandardProduct"), oManufacturerName = dr.GetOrdinal("ManufacturerName"), ... while (dr.Read()) { yield return new MyObject() { ModelNumber = dr.GetString(oModelNumber), IsStandardProduct = dr.GetBoolean(oIsStandardProduct), ManufacturerName = dr.GetString(oManufacturerName), ... }; } }` – Opus Krokus Mar 12 '13 at 14:25
  • @OpusKrokus Please update the question. Code in comments is quite hard to read and even harder to understand. Also, I think it wouldn't harm if you post `GetRelativeCost` function – J0HN Mar 12 '13 at 15:15
0

You can combine them into one line by removing the variable and calling AsParalel on result of new.

(new List<MyObject>(DB.GetAll(queryString, parameters, MyObject.Extract))).AsParallel().ForAll(s => s.RelativeCost = GetRelativeCost(s));

But, don't expect this to improve the performance of your code.

djs
  • 220
  • 1
  • 5