Remove ConcurrentDictionary from RelationalTransactionRegistry

[YuKitsune]

[sc-67483]

[shortcut-integration]

This pull request has been linked to Shortcut Story #67483: RelationalTransactionRegistry bottleneck - Requested by Luke Butters.

[borland]

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 |

[rosslovas]

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.

This is not true, it looks up by the key on Remove. This has changed from a hash lookup to a linear scan. Blocking to discuss

[adam-mccoy]

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?

[borland]

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

[rosslovas]

Sorry for the delay, didn't get a notification for your last comment. LGTM!

[borland]

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