From b1422d522be26e754e82003f2d905ebf284dfa03 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 9 Apr 2019 17:49:00 +0100 Subject: [PATCH 1/4] change SqlEnvChangePool to be simpler and safe Commit migrated from https://github.com/dotnet/corefx/commit/b9bd3548b69a14c969ecd3604ac71272ca8c4725 --- .../Data/SqlClient/TdsParserHelperClasses.cs | 91 +++---------------- 1 file changed, 14 insertions(+), 77 deletions(-) diff --git a/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs b/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs index 3b121ed1e7b626..91a3dbdff9acf3 100644 --- a/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs +++ b/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs @@ -300,103 +300,40 @@ internal void Clear() internal static class SqlEnvChangePool { - private static int _count; - private static int _capacity; - private static SqlEnvChange[] _items; -#if DEBUG - private static string[] _stacks; -#endif - - static SqlEnvChangePool() + private protected struct SqlEnvChangeWrapper { - _capacity = 10; - _items = new SqlEnvChange[_capacity]; - _count = 1; -#if DEBUG - _stacks = new string[_capacity]; -#endif + public SqlEnvChange Item; } + private static readonly SqlEnvChangeWrapper[] _items = new SqlEnvChangeWrapper[Environment.ProcessorCount * 2]; + internal static SqlEnvChange Allocate() { - SqlEnvChange retval = null; - lock (_items) + SqlEnvChangeWrapper[] items = _items; + for (int index = 0; index < items.Length; index++) { - while (_count > 0 && retval is null) + SqlEnvChange item = items[index].Item; + if (item != null && Interlocked.CompareExchange(ref items[index].Item, null, item) == item) { - int count = _count; // copy the count we think we have - int newCount = count - 1; // work out the new value we want - // exchange _count for newCount only if _count is the same as our cached copy - if (count == Interlocked.CompareExchange(ref _count, newCount, count)) - { - // count is now the previous value, we're the only thread that has it, so count-1 is safe to access - Interlocked.Exchange(ref retval, _items[count - 1]); - } - else - { - // otherwise the count wasn't what we expected, spin the while and try again. - } + return item; } } - if (retval is null) - { - retval = new SqlEnvChange(); - } - return retval; + return new SqlEnvChange(); } - internal static void Release(SqlEnvChange item, [Runtime.CompilerServices.CallerMemberName] string caller = null) + internal static void Release(SqlEnvChange item) { if (item is null) { - Debug.Fail("attenpting to release null packet"); + Debug.Fail("attenpting to release null item"); return; } item.Clear(); + SqlEnvChangeWrapper[] items = _items; + for (int index = 0; index < items.Length && Interlocked.CompareExchange(ref items[index].Item, item, null) != null; index++) { -#if DEBUG - if (_count > 0) - { - for (int index = 0; index < _count; index += 1) - { - if (object.ReferenceEquals(item, _items[index])) - { - Debug.Assert(false,$"releasing an item which already exists in the pool, count={_count}, index={index}, caller={caller}, released at={_stacks[index]}"); - } - } - } -#endif - int tries = 0; - - while (!(item is null) && tries < 3 && _count < _capacity) - { - int count = _count; - int newCount = count + 1; - if (count == Interlocked.CompareExchange(ref _count, newCount, count)) - { - _items[count] = item; -#if DEBUG - _stacks[count] = caller; -#endif - item = null; - } - else - { - tries += 1; - } - } } } - - internal static int Count => _count; - - internal static int Capacity => _capacity; - - internal static void Clear() - { - Array.Clear(_items, 0, _capacity); - _count = 0; - } } internal sealed class SqlLogin From 0902c2ea8c0a11eb50d982de46f66daca1b03105 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 9 Apr 2019 19:29:45 +0100 Subject: [PATCH 2/4] address feedback Commit migrated from https://github.com/dotnet/corefx/commit/73885393cea96b536564ea39f09a86d6494d83a0 --- .../Data/SqlClient/TdsParserHelperClasses.cs | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs b/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs index 91a3dbdff9acf3..1d8f6a77b7becf 100644 --- a/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs +++ b/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs @@ -298,18 +298,26 @@ internal void Clear() } } + /// + /// this class implements a static pool of SqlEnvChange objects to help reduce memory churn in low resource + /// situations. For low frequency querying behaviour most env change objects will be sourced and returned + /// to the pool. In higher frequency situations the pool will create and discard any additional objects that + /// are needed without blocking the caller so that it doesn't become a bottleneck, overall memory consumption + /// should still be lower than without the pool + /// internal static class SqlEnvChangePool { - private protected struct SqlEnvChangeWrapper + // the conatiner struct allows avoidance of array variance checks on assignment + private protected struct SqlEnvChangeContainer { public SqlEnvChange Item; } - private static readonly SqlEnvChangeWrapper[] _items = new SqlEnvChangeWrapper[Environment.ProcessorCount * 2]; + private static readonly SqlEnvChangeContainer[] _items = new SqlEnvChangeContainer[Environment.ProcessorCount * 2]; internal static SqlEnvChange Allocate() { - SqlEnvChangeWrapper[] items = _items; + SqlEnvChangeContainer[] items = _items; for (int index = 0; index < items.Length; index++) { SqlEnvChange item = items[index].Item; @@ -318,6 +326,8 @@ internal static SqlEnvChange Allocate() return item; } } + // if we didn't find an item in the pool we've either never craete any or we're under pressure + // so just create a new one and let the caller get on with their work return new SqlEnvChange(); } @@ -325,14 +335,19 @@ internal static void Release(SqlEnvChange item) { if (item is null) { - Debug.Fail("attenpting to release null item"); + Debug.Fail("attempting to release null item"); return; } item.Clear(); - SqlEnvChangeWrapper[] items = _items; - for (int index = 0; index < items.Length && Interlocked.CompareExchange(ref items[index].Item, item, null) != null; index++) + SqlEnvChangeContainer[] items = _items; + for (int index = 0; index < items.Length; index++) { + if (Interlocked.CompareExchange(ref items[index].Item, item, null) is null) + { + break; + } } + // if we didn't add the item to the cache just let the go to the GC } } From 270b198ba4fb4bd008fec2dce48ceb0b5698baa3 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 9 Apr 2019 20:07:08 +0100 Subject: [PATCH 3/4] spelling feedback Commit migrated from https://github.com/dotnet/corefx/commit/f27d235b0bfc08106258c7e3ffff49d3811d2cae --- .../src/System/Data/SqlClient/TdsParserHelperClasses.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs b/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs index 1d8f6a77b7becf..ef8a0fa46216e8 100644 --- a/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs +++ b/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs @@ -326,7 +326,7 @@ internal static SqlEnvChange Allocate() return item; } } - // if we didn't find an item in the pool we've either never craete any or we're under pressure + // if we didn't find an item in the pool we've either never create any or we're under pressure // so just create a new one and let the caller get on with their work return new SqlEnvChange(); } @@ -347,7 +347,7 @@ internal static void Release(SqlEnvChange item) break; } } - // if we didn't add the item to the cache just let the go to the GC + // if we didn't add the item to the cache just let it go to the GC } } From 32daacbaf948112b5dc193cedebedc01c32d7b43 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 9 Apr 2019 21:19:29 +0100 Subject: [PATCH 4/4] remove pool Commit migrated from https://github.com/dotnet/corefx/commit/538cf16de9aa7155d6b905272648f3d9a3492cb0 --- .../src/System/Data/SqlClient/TdsParser.cs | 4 +- .../Data/SqlClient/TdsParserHelperClasses.cs | 53 ------------------- 2 files changed, 2 insertions(+), 55 deletions(-) diff --git a/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs b/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs index 8e07c39cca1f4a..2c35b0de3a531c 100644 --- a/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs +++ b/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs @@ -1937,7 +1937,7 @@ internal bool TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataRead } SqlEnvChange head = env; env = env.Next; - SqlEnvChangePool.Release(head); + head.Clear(); head = null; } break; @@ -2187,7 +2187,7 @@ private bool TryProcessEnvChange(int tokenLength, TdsParserStateObject stateObj, while (tokenLength > processedLength) { - SqlEnvChange env = SqlEnvChangePool.Allocate(); + var env = new SqlEnvChange(); if (!stateObj.TryReadByte(out env.type)) { diff --git a/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs b/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs index ef8a0fa46216e8..96e1895c0ab888 100644 --- a/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs +++ b/src/libraries/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs @@ -298,59 +298,6 @@ internal void Clear() } } - /// - /// this class implements a static pool of SqlEnvChange objects to help reduce memory churn in low resource - /// situations. For low frequency querying behaviour most env change objects will be sourced and returned - /// to the pool. In higher frequency situations the pool will create and discard any additional objects that - /// are needed without blocking the caller so that it doesn't become a bottleneck, overall memory consumption - /// should still be lower than without the pool - /// - internal static class SqlEnvChangePool - { - // the conatiner struct allows avoidance of array variance checks on assignment - private protected struct SqlEnvChangeContainer - { - public SqlEnvChange Item; - } - - private static readonly SqlEnvChangeContainer[] _items = new SqlEnvChangeContainer[Environment.ProcessorCount * 2]; - - internal static SqlEnvChange Allocate() - { - SqlEnvChangeContainer[] items = _items; - for (int index = 0; index < items.Length; index++) - { - SqlEnvChange item = items[index].Item; - if (item != null && Interlocked.CompareExchange(ref items[index].Item, null, item) == item) - { - return item; - } - } - // if we didn't find an item in the pool we've either never create any or we're under pressure - // so just create a new one and let the caller get on with their work - return new SqlEnvChange(); - } - - internal static void Release(SqlEnvChange item) - { - if (item is null) - { - Debug.Fail("attempting to release null item"); - return; - } - item.Clear(); - SqlEnvChangeContainer[] items = _items; - for (int index = 0; index < items.Length; index++) - { - if (Interlocked.CompareExchange(ref items[index].Item, item, null) is null) - { - break; - } - } - // if we didn't add the item to the cache just let it go to the GC - } - } - internal sealed class SqlLogin { internal int timeout; // login timeout