Nevermore icon indicating copy to clipboard operation
Nevermore copied to clipboard

Remove ConcurrentDictionary from RelationalTransactionRegistry

Open YuKitsune opened this issue 1 year ago • 3 comments
trafficstars

[sc-67483]

YuKitsune avatar Feb 12 '24 05:02 YuKitsune

Adam and I jumped on this and tweaked it to use a List instead of Dictionary, as it never looked up anything by the dictionary keys anyway.

Ad-hoc BenchmarkDotnet results

baseline on master:

|                       Method |     Mean |   Error |  StdDev |
|----------------------------- |---------:|--------:|--------:|
| AddTransactionWithoutLogging | 154.6 ms | 2.84 ms | 4.82 ms |

Code in this PR:

|                       Method |     Mean |     Error |    StdDev |   Median |
|----------------------------- |---------:|----------:|----------:|---------:|
| AddTransactionWithoutLogging | 6.444 ms | 0.1285 ms | 0.3003 ms | 6.295 ms |

borland avatar Mar 04 '24 03:03 borland

I've added some benchmarks and tested using a few collection types. Below are the results.

5,000 transaction ("Realistic") 100,000 transactions ("Extreme")
ConcurrentDictionary Add 53.45ms 1280.43ms
Remove 0.127ms 5.806ms
List Add 0.869ms 43.681ms
Remove 1.098ms 519.403ms
HashSet Add 1.307ms 81.638ms
Remove 0.124ms 3.529ms
  • The difference between List<T> and HashSet<T> at a "realistic" transaction count (5,000) is insignificant
  • Both allow adding much more quickly than ConcurrentDictionary<TKey, TValue>
  • With a very large count, HashSet<T> performs significantly better (~175x) with removals, while only performing slightly worse (~2x) on adds

Based on the above findings, I've gone with HashSet<T>. While I don't think the decision is going to make a noticeable difference in almost all cases, the potential performance trade-off is more favourable with HashSet<T>.

@rosslovas could you please take a look at the above and let me know if you agree?

adam-mccoy avatar Mar 25 '24 23:03 adam-mccoy

I've looked at the code+benchmarks and run them locally as well, I get similar results to Adam. I think we should merge the HashSet change in

borland avatar Apr 23 '24 19:04 borland

Fixes https://github.com/OctopusDeploy/ResearchAndDevelopment/issues/667

borland avatar Apr 26 '24 05:04 borland