| Bug #34001 | Connection pooling has too much unnecessary contention | ||
|---|---|---|---|
| Submitted: | 23 Jan 2008 1:56 | Modified: | 17 Nov 2008 23:36 |
| Reporter: | Maxim Mass | Email Updates: | |
| Status: | Closed | Impact on me: | |
| Category: | Connector / NET | Severity: | S5 (Performance) |
| Version: | 5.1.4 | OS: | Any |
| Assigned to: | CPU Architecture: | Any | |
[23 Jan 2008 2:06]
Maxim Mass
Note that pinging is removed from this code but could easily be added back in.. From our tests pinging often times doubled the amount of time spent executing a query and we removed it as we didn't see the point of doing it.
[21 Apr 2008 8:29]
Tonci Grgin
Maxim, if you agree, I will close this report and add a note to Bug#36194 that it should be checked together with this one.
[21 Apr 2008 19:06]
Maxim Mass
Tonci, that's not a problem for me -- whatever makes it easier to have the issues resolved one way or another. BTW, I idle in #mysql and #mysql-dev on freenode if you'd like to reach me under nick MaxM.
[22 Apr 2008 12:17]
Tonci Grgin
Maxim, I believe this is a part of ongoing work in c/NET 5.2 and will not be back-ported to 5.1 branch. If I'm wrong, Reggie will correct me. I wish I had time for free node, but I don't...
[24 Apr 2008 19:30]
Reggie Burnett
Maxim
I this this patch is still a bit buggy. Consider this part:
// wait till we are allowed in
if (Interlocked.Decrement(ref counter) < 0) {
int ticks = (int) settings.ConnectionTimeout * 1000;
bool allowed = poolGate.WaitOne(ticks, false);
if (!allowed)
throw new MySqlException(Resources.TimeoutGettingConnection);
}
Let's assume you have a pool with 20 connections. Since you are not locking the entire object it is entirely possible that 20 threads could hit this method and execute the decrement before any of them executed the compare. This would cause all the threads to block on poolGate incorrectly. If I am misunderstanding your patch please let me know.
[7 May 2008 13:47]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/46456
[7 May 2008 13:48]
Reggie Burnett
Patch applied in 5.2.2
[16 May 2008 13:10]
MC Brown
A note has been added to the 5.2.2 changelog : There was a high level of contention in the connection pooling code that could lead to delays when opening connections and submitting queries. The connection pooling code has been modified to try and limit the effects of the contention issue.
[13 Nov 2008 0:52]
d di
Maxim, has the bug addressed in comment #[24 Apr 21:30] been resolved or is it still in the committed patch?
[17 Nov 2008 23:36]
Maxim Mass
David, I may be taking Reggie's concern out of context but looking at that code snippet I don't see the issue. Twenty threads hitting the decrement and context switching before the comparison would each get a different counter and the comparison will work regardless. Either way, the patch that Reggie submitted looks sufficiently different but similar in concept that it's likely a non-issue. I'm just glad it's fixed one way or another! :)
Description: The locking strategy in MySqlPool and MySqlPoolManager leads to too much contention which occasionally causes significant performance drops. This is an attempt to fine tune the locking strategy in order to keep an application running smoothly while pooled connections are being created, reused, and destroyed. How to repeat: MySqlPoolManager unnecessarily locks on the entire list of pools when a RemoveConnection or ReleaseConnection is being performed on a given pool itself. MySqlPool is changed to only use the semaphore when necessary as it incurs a significant cpu hit even when it doesn't block. More granular locking is done to reduce contention. Please note that this is part of a series of MySql.Data.DLL optimizations made by MindTouch for open source wiki software Deki Wiki. Suggested fix: MySqlPoolManager: Move these calls outside of the lock on global pools hash pool.RemoveConnection(driver); and pool.ReleaseConnection(driver); MySqlPool: internal sealed class MySqlPool { #if NET20 private List<Driver> inUsePool; private Queue<Driver> idlePool; #else private ArrayList inUsePool; private Queue idlePool; #endif private MySqlConnectionStringBuilder settings; private uint minSize; private uint maxSize; private ProcedureCache procedureCache; private Object lockObject; private Semaphore poolGate; private int counter; public MySqlPool(MySqlConnectionStringBuilder settings) { minSize = settings.MinimumPoolSize; maxSize = settings.MaximumPoolSize; if (minSize > maxSize) minSize = maxSize; this.settings = settings; #if NET20 inUsePool = new List<Driver>((int) maxSize); idlePool = new Queue<Driver>((int) maxSize); #else inUsePool =new ArrayList((int)maxSize); idlePool = new Queue((int)maxSize); #endif // prepopulate the idle pool to minSize for (int i = 0; i < minSize; i++) idlePool.Enqueue(Driver.Create(settings)); procedureCache = new ProcedureCache((int) settings.ProcedureCacheSize); poolGate = new Semaphore(0, int.MaxValue); counter = (int) maxSize; // we don't really need to create this but it makes the code a bit cleaner lockObject = new Object(); } #region Properties public MySqlConnectionStringBuilder Settings { get { return settings; } set { settings = value; } } public ProcedureCache ProcedureCache { get { return procedureCache; } } #endregion /// <summary> /// CheckoutConnection handles the process of pulling a driver /// from the idle pool, possibly resetting its state, /// and adding it to the in use pool. We assume that this method is only /// called inside an active lock so there is no need to acquire a new lock. /// </summary> /// <returns>An idle driver object</returns> /// <summary> /// It is assumed that this method is only called from inside an active lock. /// </summary> private Driver GetPooledConnection() { // check if an idle connection exists // if yes, take it // if not, create one and add it to the "inuse" connection list Driver d = null; lock (lockObject) { if (idlePool.Count > 0) { d = (Driver) idlePool.Dequeue(); } } if (d == null) { d = Driver.Create(settings); } else { if (settings.ConnectionReset) d.Reset(); } lock (lockObject) { inUsePool.Add(d); } return d; } public void ReleaseConnection(Driver driver) { lock (lockObject) { inUsePool.Remove(driver); } if (driver.IsTooOld()) { driver.Close(); } else { lock (lockObject) { idlePool.Enqueue(driver); } } // we now either have a connection available or have room to make // one so we release one slot in our semaphore if (Interlocked.Increment(ref counter) <= 0) poolGate.Release(); } /// <summary> /// Removes a connection from the in use pool. The only situations where this method /// would be called are when a connection that is in use gets some type of fatal exception /// or when the connection is being returned to the pool and it's too old to be /// returned. /// </summary> /// <param name="driver"></param> public void RemoveConnection(Driver driver) { bool release; lock (lockObject) { release = inUsePool.Remove(driver); } if (release) { if (Interlocked.Increment(ref counter) <= 0) poolGate.Release(); } } public Driver GetConnection() { // wait till we are allowed in if (Interlocked.Decrement(ref counter) < 0) { int ticks = (int) settings.ConnectionTimeout * 1000; bool allowed = poolGate.WaitOne(ticks, false); if (!allowed) throw new MySqlException(Resources.TimeoutGettingConnection); } // if we get here, then it means that we either have an idle connection // or room to make a new connection try { Driver d = GetPooledConnection(); return d; } catch (Exception ex) { if (settings.Logging) Logger.LogException(ex); if (Interlocked.Increment(ref counter) <= 0) poolGate.Release(); throw; } } }