Contention in NpgsqlConnection.GetPoolAndSettings

[shortspider]

Steps to reproduce

Run 15000 queries/second on a database with multiple connection pools. CPU will be abnormally high due to contention on a lock to get connection pools.

The issue

On one of our highest traffic databases we have a number of connection pools. In tracking down some recent high CPU issues with perfview we found the following:

What this shows is that ~64% of CPU time was being spent contending for a lock in NpgsqlConnection.GetPoolAndSettings. Looking at that method I can see it's a global (process wide) lock around the dictionary of connection strings to connection pools.

I believe this can be improved by using a ConcurrentDictionary instead of a normal dictionary and a lock. The ConcurrentDictionary will allow for lockless reads (so TryGetValue will take no locks) and when adding/updating a value only the one spicific key will be locked, so traffic to get pool A will not affect people trying to get pool B.

What do you think? If you agree I'd like to submit a PR to implement this.

Further technical details

Npgsql version: 3.2.5
PostgreSQL version: 9.6.2
Operating system: Windows Server 2012 R2

[roji]

That makes perfect sense, I'm honestly not sure why it's a dictionary with locking rather than a ConcurrentDictionary. This comes at a very good time - performance is the major focus of 3.3, and lots of improvements have been committed (and more are planned).

Please go ahead and submit a PR on this. Can you please also post the results of the benchmark ConnectionOpenCloseBenchmarks before and after your change?

[roji]

@damageboy, you may be interested in this.

[shortspider]

@roji The summary of benchmark ConnectionOpenCloseBenchmarks are attached. First is dev branch, second is my branch.

npgsql.default.txt

npgsql.concurrentpool.txt

[roji]

Interestingly, the benchmarks are quite inconclusive. The scenario being tested is a massively read-only load with a single database. I expect this to be the typical Npgsql scenario, with only one database (or a very small number) being contacted, and with the pool dictionary being 100% read-only after an initial population phase at application startup.

I think this still makes sense, although the review comments in #1753 still need to be addressed.