Nevermore
Nevermore copied to clipboard
Remove ConcurrentDictionary from RelationalTransactionRegistry
[sc-67483]
This pull request has been linked to Shortcut Story #67483: RelationalTransactionRegistry bottleneck - Requested by Luke Butters.
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 |
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>andHashSet<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?
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
Fixes https://github.com/OctopusDeploy/ResearchAndDevelopment/issues/667