1

Which one is faster?

Im doing tests on a ConcurrentDictionary and want to randomize it and just return 1 result. Essentially pick a random result from it.

Which method is faster/more efficient? Efficiency as in, as little cpu/memory while having low possible errors like conflicts and such.

Method A.

Random rand = new Random();
var result = concDict.ElementAt(rand.Next(concDict.Count() - 1));

Method B.

var result = concDict.OrderBy(x=>Guid.NewGuid()).First();

In my "vague" testing I dont see much difference apart from Method B being more efficient. Method A can succum to the concDict's Count being out of sync with the concDict.ElementAt causing ArgumentOutOfRangeException: 'maxValue' must be greater than zero. while Method B literally cant cause that.

Ma Dude
  • 477
  • 1
  • 5
  • 17
  • 5
    If you ask which is more efficient for different parameters - test it. Check execution time, check cpu usage, check memory.... Google something like "how to test code performance c#" – Gilad Green Dec 05 '17 at 07:27
  • 3
    Why would ordering with creating a GUID for each element be more efficient than accessing an element at a specific index? Seems a bit unlikely. – ViRuSTriNiTy Dec 05 '17 at 07:28
  • 3
    Before you ask which one is faster you should ask which one is correct. Guid.NewGuid is guaranteed to return a globally unique id not one with any particularly good randomness properties. – Mike Zboray Dec 05 '17 at 07:28
  • @ViRuSTriNiTy because the concDict.Count() will get the count for changes that happened sometime after/before so the concDict.ElementAt will be slightly off sync, thee count will be lets say 300 while the concDict.ElementAt bit would have around 240 items causing a ArgumentException. – Ma Dude Dec 05 '17 at 07:30
  • @mikez Well since my needs are to just pick 1 random entry, it really doesnt matter how random it is aslong as it is randomized. – Ma Dude Dec 05 '17 at 07:31
  • 2
    [race your horses.](https://ericlippert.com/2012/12/17/performance-rant/) Also, does it really matter? unless using this in a tight loop, the performance difference will probably be so small it would be irrelevant. – Zohar Peled Dec 05 '17 at 07:32
  • 1
    @ImPRAGMA How do you know that sorting by guids generated from Guid.NewGuid is guaranteed to give you a random item from the collection? If it does that it is not a guarantee of that method. This kind of "off label" use is ill-advised. Use a method that is documented to return a random value like Random.Next. – Mike Zboray Dec 05 '17 at 07:35
  • There's no *guarantee* given in the documentation that `OrderBy` performs key extraction only once for each element in the set. Given that your key extraction function is non-deterministic, that *could* mean that the ordering function never terminates (since two elements could continually return keys that indicate that they're always out of sequence) – Damien_The_Unbeliever Dec 05 '17 at 07:37
  • Note that `OrderBy` is subject to the same problem as `ElementAt` + Count (can throw exception if you modify collection during ordering): https://stackoverflow.com/q/47630824/5311735 – Evk Dec 05 '17 at 07:37
  • 2
    Apart from the question whether `Guid.NewGuid()` will produce evenly distributed random numbers (I’m actually not sure why you aren’t using `Random` there too), the efficiency of these should be somewhat clear: In A, you are accessing a single random element from the sequence. This is done in constant time. Method B however requires you to *sort* the whole sequence. This will be O(n log n) which is definitely worse, especially when you just want to get one element out of it. – If you need just one item, pick one from a random spot; if you need multiple, shuffle the sequence in linear time. – poke Dec 05 '17 at 07:57
  • The `OrderBy` solution btw. avoids concurrency issues because it is effectively creating a copy at the time the ordering starts. So by the time the ordering is done, it may even yield an element that is no longer part of the original collection. But if that’s acceptable to you, of course you could just create a copy in method A as well and still pick a random element from it. – poke Dec 05 '17 at 07:59
  • @Evk Actually thats why I use concurrentDictionary to mitigate that issue :P I actually found out why it was returning an exception, if the concDict only had 1 item, it would -1 it returning 0 lol – Ma Dude Dec 05 '17 at 08:00
  • @poke Oh I didnt know that :D Thanks! I decided to stick with ElementTo it makes a lot more sense since I sorted out my issue :D – Ma Dude Dec 05 '17 at 08:01
  • But concurrent dictionary doesn't mitigate that issue. Your exact code may throw on `OrderBy` if you will add items to concurrent dictionary from another threads at the same time. – Evk Dec 05 '17 at 08:02

0 Answers0