From 5bcbf0a6a51599d9aceb8739d0688fedd12f438a Mon Sep 17 00:00:00 2001 From: stephentoub Date: Sun, 27 Mar 2016 13:56:09 -0400 Subject: [PATCH 1/2] Reduce allocations in CookieContainer.GetCookieHeader HttpClient defaults to UseCookies=true, which means by default every request ends up asking the CookieContainer to get the cookie header that should be used. Today, even if there aren't any cookies for the Uri, this ends up allocating a CookieCollection, a StringBuilder, a couple of Lists, an iterator, etc., none of which are actually necessary. And if there are cookies for the Uri, the serialization of each cookie results in a handful of strings, char[]s, and string[] allocations. This commit fixes the implementation to avoid these. (There are still some that could be cleaned up with further work, but it becomes much more invasive.) --- .../src/System.Net.Primitives.csproj | 3 + .../src/System/Net/Cookie.cs | 127 +++++++++--------- .../src/System/Net/CookieContainer.cs | 51 ++++--- ...stem.Net.Primitives.UnitTests.Tests.csproj | 3 + 4 files changed, 103 insertions(+), 81 deletions(-) diff --git a/src/System.Net.Primitives/src/System.Net.Primitives.csproj b/src/System.Net.Primitives/src/System.Net.Primitives.csproj index d440dc656a23..849cd46b5d78 100644 --- a/src/System.Net.Primitives/src/System.Net.Primitives.csproj +++ b/src/System.Net.Primitives/src/System.Net.Primitives.csproj @@ -91,6 +91,9 @@ Common\System\Net\NetworkInformation\HostInformation.cs + + Common\System\IO\StringBuilderCache.cs + Common\System\Net\Shims\TraceSource.cs diff --git a/src/System.Net.Primitives/src/System/Net/Cookie.cs b/src/System.Net.Primitives/src/System/Net/Cookie.cs index 685216233216..d5f2eca4c752 100644 --- a/src/System.Net.Primitives/src/System/Net/Cookie.cs +++ b/src/System.Net.Primitives/src/System/Net/Cookie.cs @@ -2,10 +2,11 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -using System.Collections; using System.Collections.Generic; using System.Diagnostics; using System.Globalization; +using System.IO; +using System.Text; using System.Threading; // The NETNative_SystemNetHttp #define is used in some source files to indicate we are compiling classes @@ -181,19 +182,6 @@ public string Domain } } - private string _Domain - { - get - { - return (Plain || _domainImplicit || (_domain.Length == 0)) - ? string.Empty - : (SpecialAttributeLiteral - + DomainAttributeName - + EqualsLiteral + (IsQuotedDomain ? "\"" : string.Empty) - + _domain + (IsQuotedDomain ? "\"" : string.Empty)); - } - } - internal bool DomainImplicit { get @@ -272,19 +260,6 @@ public string Path } } - private string _Path - { - get - { - return (Plain || _pathImplicit || (_path.Length == 0)) - ? string.Empty - : (SpecialAttributeLiteral - + PathAttributeName - + EqualsLiteral - + _path); - } - } - internal bool Plain { get @@ -666,17 +641,6 @@ internal int[] PortList } } - private string _Port - { - get - { - return _portImplicit ? string.Empty : - (SpecialAttributeLiteral - + PortAttributeName - + ((_port.Length == 0) ? string.Empty : (EqualsLiteral + _port))); - } - } - public bool Secure { get @@ -763,18 +727,6 @@ public int Version } } - private string _Version - { - get - { - return (Version == 0) ? string.Empty : - (SpecialAttributeLiteral - + VersionAttributeName - + EqualsLiteral + (IsQuotedVersion ? "\"" : string.Empty) - + _version.ToString(NumberFormatInfo.InvariantInfo) + (IsQuotedVersion ? "\"" : string.Empty)); - } - } - public override bool Equals(object comparand) { Cookie other = comparand as Cookie; @@ -794,22 +746,65 @@ public override int GetHashCode() public override string ToString() { - string domain = _Domain; - string path = _Path; - string port = _Port; - string version = _Version; - - string result = - ((version.Length == 0) ? string.Empty : (version + SeparatorLiteral)) - + Name + EqualsLiteral + Value - + ((path.Length == 0) ? string.Empty : (SeparatorLiteral + path)) - + ((domain.Length == 0) ? string.Empty : (SeparatorLiteral + domain)) - + ((port.Length == 0) ? string.Empty : (SeparatorLiteral + port)); - if (result == "=") - { - return string.Empty; - } - return result; + StringBuilder sb = StringBuilderCache.Acquire(); + ToString(sb); + return StringBuilderCache.GetStringAndRelease(sb); + } + + internal void ToString(StringBuilder sb) + { + int beforeLength = sb.Length; + + // Add the Cookie version if necessary. + if (Version != 0) + { + sb.Append(SpecialAttributeLiteral + VersionAttributeName + EqualsLiteral); // const strings + if (IsQuotedVersion) sb.Append('"'); + sb.Append(_version.ToString(NumberFormatInfo.InvariantInfo)); + if (IsQuotedVersion) sb.Append('"'); + sb.Append(SeparatorLiteral); + } + + // Add the Cookie Name=Value pair. + sb.Append(Name).Append(EqualsLiteral).Append(Value); + + if (!Plain) + { + // Add the Path if necessary. + if (!_pathImplicit && _path.Length > 0) + { + sb.Append(SeparatorLiteral + SpecialAttributeLiteral + PathAttributeName + EqualsLiteral); // const strings + sb.Append(_path); + } + + // Add the Domain if necessary. + if (!_domainImplicit && _domain.Length > 0) + { + sb.Append(SeparatorLiteral + SpecialAttributeLiteral + DomainAttributeName + EqualsLiteral); // const strings + if (IsQuotedDomain) sb.Append('"'); + sb.Append(_domain); + if (IsQuotedDomain) sb.Append('"'); + } + } + + // Add the Port if necessary. + if (!_portImplicit) + { + sb.Append(SeparatorLiteral + SpecialAttributeLiteral + PortAttributeName); // const strings + if (_port.Length > 0) + { + sb.Append(EqualsLiteral); + sb.Append(_port); + } + } + + // Check to see whether the only thing we added was "=", and if so, + // remove it so that we leave the StringBuilder unchanged in contents. + int afterLength = sb.Length; + if (afterLength == (1 + beforeLength) && sb[beforeLength] == '=') + { + sb.Length = beforeLength; + } } internal string ToServerString() diff --git a/src/System.Net.Primitives/src/System/Net/CookieContainer.cs b/src/System.Net.Primitives/src/System/Net/CookieContainer.cs index 168158c78aa9..87d38238cdde 100644 --- a/src/System.Net.Primitives/src/System/Net/CookieContainer.cs +++ b/src/System.Net.Primitives/src/System/Net/CookieContainer.cs @@ -2,9 +2,9 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -using System.Collections; using System.Collections.Generic; using System.Diagnostics; +using System.IO; using System.Net.NetworkInformation; using System.Text; @@ -728,17 +728,17 @@ public CookieCollection GetCookies(Uri uri) { throw new ArgumentNullException(nameof(uri)); } - return InternalGetCookies(uri); + return InternalGetCookies(uri) ?? new CookieCollection(); } internal CookieCollection InternalGetCookies(Uri uri) { bool isSecure = (uri.Scheme == UriScheme.Https); int port = uri.Port; - CookieCollection cookies = new CookieCollection(); + CookieCollection cookies = null; List domainAttributeMatchAnyCookieVariant = new List(); - List domainAttributeMatchOnlyCookieVariantPlain = new List(); + List domainAttributeMatchOnlyCookieVariantPlain = null; string fqdnRemote = uri.Host; @@ -780,6 +780,11 @@ internal CookieCollection InternalGetCookies(Uri uri) { while ((dot < last) && (dot = fqdnRemote.IndexOf('.', dot + 1)) != -1) { + if (domainAttributeMatchOnlyCookieVariantPlain == null) + { + domainAttributeMatchOnlyCookieVariantPlain = new List(); + } + // These candidates can only match CookieVariant.Plain cookies. domainAttributeMatchOnlyCookieVariantPlain.Add(fqdnRemote.Substring(dot)); } @@ -787,13 +792,16 @@ internal CookieCollection InternalGetCookies(Uri uri) } } - BuildCookieCollectionFromDomainMatches(uri, isSecure, port, cookies, domainAttributeMatchAnyCookieVariant, false); - BuildCookieCollectionFromDomainMatches(uri, isSecure, port, cookies, domainAttributeMatchOnlyCookieVariantPlain, true); + BuildCookieCollectionFromDomainMatches(uri, isSecure, port, ref cookies, domainAttributeMatchAnyCookieVariant, false); + if (domainAttributeMatchOnlyCookieVariantPlain != null) + { + BuildCookieCollectionFromDomainMatches(uri, isSecure, port, ref cookies, domainAttributeMatchOnlyCookieVariantPlain, true); + } return cookies; } - private void BuildCookieCollectionFromDomainMatches(Uri uri, bool isSecure, int port, CookieCollection cookies, List domainAttribute, bool matchOnlyPlainCookie) + private void BuildCookieCollectionFromDomainMatches(Uri uri, bool isSecure, int port, ref CookieCollection cookies, List domainAttribute, bool matchOnlyPlainCookie) { for (int i = 0; i < domainAttribute.Count; i++) { @@ -819,7 +827,7 @@ private void BuildCookieCollectionFromDomainMatches(Uri uri, bool isSecure, int CookieCollection cc = pair.Value; cc.TimeStamp(CookieCollection.Stamp.Set); - MergeUpdateCollections(cookies, cc, port, isSecure, matchOnlyPlainCookie); + MergeUpdateCollections(ref cookies, cc, port, isSecure, matchOnlyPlainCookie); if (path == "/") { @@ -840,7 +848,7 @@ private void BuildCookieCollectionFromDomainMatches(Uri uri, bool isSecure, int if (cc != null) { cc.TimeStamp(CookieCollection.Stamp.Set); - MergeUpdateCollections(cookies, cc, port, isSecure, matchOnlyPlainCookie); + MergeUpdateCollections(ref cookies, cc, port, isSecure, matchOnlyPlainCookie); } } @@ -856,11 +864,11 @@ private void BuildCookieCollectionFromDomainMatches(Uri uri, bool isSecure, int } } - private void MergeUpdateCollections(CookieCollection destination, CookieCollection source, int port, bool isSecure, bool isPlainOnly) + private void MergeUpdateCollections(ref CookieCollection destination, CookieCollection source, int port, bool isSecure, bool isPlainOnly) { lock (source) { - // Cannot use foreach as we going update 'source' + // Cannot use foreach as we are going to update 'source' for (int idx = 0; idx < source.Count; ++idx) { bool to_add = false; @@ -910,6 +918,10 @@ private void MergeUpdateCollections(CookieCollection destination, CookieCollecti // In 'source' are already ordered. // If two same cookies come from different 'source' then they // will follow (not replace) each other. + if (destination == null) + { + destination = new CookieCollection(); + } destination.InternalAdd(cookie, false); } } @@ -930,21 +942,30 @@ public string GetCookieHeader(Uri uri) internal string GetCookieHeader(Uri uri, out string optCookie2) { CookieCollection cookies = InternalGetCookies(uri); + if (cookies == null) + { + optCookie2 = string.Empty; + return string.Empty; + } + string delimiter = string.Empty; - var builder = new StringBuilder(); - foreach (Cookie cookie in cookies) + var builder = StringBuilderCache.Acquire(); + for (int i = 0; i < cookies.Count; i++) { - builder.Append(delimiter).Append(cookie.ToString()); + builder.Append(delimiter); + cookies[i].ToString(builder); + delimiter = "; "; } + optCookie2 = cookies.IsOtherVersionSeen ? (Cookie.SpecialAttributeLiteral + Cookie.VersionAttributeName + Cookie.EqualsLiteral + Cookie.MaxSupportedVersionString) : string.Empty; - return builder.ToString(); + return StringBuilderCache.GetStringAndRelease(builder); } public void SetCookies(Uri uri, string cookieHeader) diff --git a/src/System.Net.Primitives/tests/UnitTests/System.Net.Primitives.UnitTests.Tests.csproj b/src/System.Net.Primitives/tests/UnitTests/System.Net.Primitives.UnitTests.Tests.csproj index 5c21fb3a05c8..2a9094bedf89 100644 --- a/src/System.Net.Primitives/tests/UnitTests/System.Net.Primitives.UnitTests.Tests.csproj +++ b/src/System.Net.Primitives/tests/UnitTests/System.Net.Primitives.UnitTests.Tests.csproj @@ -88,6 +88,9 @@ ProductionCode\Common\System\Net\UriScheme.cs + + Common\System\IO\StringBuilderCache.cs + ProductionCode\Common\System\Net\Shims\TraceSource.cs From 01e5c74e5d671ebb65035bc83a556ac9760d0a70 Mon Sep 17 00:00:00 2001 From: stephentoub Date: Mon, 28 Mar 2016 22:46:55 -0400 Subject: [PATCH 2/2] Port Cookie changes to Http's cookie.cs Per PR feedback. --- .../src/netcore50/System/Net/cookie.cs | 125 ++++++++---------- 1 file changed, 58 insertions(+), 67 deletions(-) diff --git a/src/System.Net.Http/src/netcore50/System/Net/cookie.cs b/src/System.Net.Http/src/netcore50/System/Net/cookie.cs index c9b928250150..c1b51f0d7ce9 100644 --- a/src/System.Net.Http/src/netcore50/System/Net/cookie.cs +++ b/src/System.Net.Http/src/netcore50/System/Net/cookie.cs @@ -5,6 +5,7 @@ using System.Collections; using System.Collections.Generic; using System.Globalization; +using System.Text; using System.Threading; // The NETNative_SystemNetHttp #define is used in some source files to indicate we are compiling classes @@ -221,20 +222,6 @@ public string Domain } } - private string _Domain - { - get - { - return (Plain || m_domain_implicit || (m_domain.Length == 0)) - ? string.Empty - : (SpecialAttributeLiteral - + DomainAttributeName - + EqualsLiteral + (IsQuotedDomain ? "\"" : string.Empty) - + m_domain + (IsQuotedDomain ? "\"" : string.Empty) - ); - } - } - internal bool DomainImplicit { get @@ -325,20 +312,6 @@ public string Path } } - private string _Path - { - get - { - return (Plain || m_path_implicit || (m_path.Length == 0)) - ? string.Empty - : (SpecialAttributeLiteral - + PathAttributeName - + EqualsLiteral - + m_path - ); - } - } - internal bool Plain { get @@ -729,18 +702,6 @@ internal int[] PortList } } - private string _Port - { - get - { - return m_port_implicit ? string.Empty : - (SpecialAttributeLiteral - + PortAttributeName - + ((m_port.Length == 0) ? string.Empty : (EqualsLiteral + m_port)) - ); - } - } - /// /// [To be supplied.] /// @@ -834,18 +795,6 @@ public int Version } } - private string _Version - { - get - { - return (Version == 0) ? string.Empty : - (SpecialAttributeLiteral - + VersionAttributeName - + EqualsLiteral + (IsQuotedVersion ? "\"" : string.Empty) - + m_version.ToString(NumberFormatInfo.InvariantInfo) + (IsQuotedVersion ? "\"" : string.Empty)); - } - } - // methods @@ -897,23 +846,65 @@ public override int GetHashCode() /// public override string ToString() { - string domain = _Domain; - string path = _Path; - string port = _Port; - string version = _Version; - - string result = - ((version.Length == 0) ? string.Empty : (version + SeparatorLiteral)) - + Name + EqualsLiteral + Value - + ((path.Length == 0) ? string.Empty : (SeparatorLiteral + path)) - + ((domain.Length == 0) ? string.Empty : (SeparatorLiteral + domain)) - + ((port.Length == 0) ? string.Empty : (SeparatorLiteral + port)) - ; - if (result == "=") + var sb = new StringBuilder(); + ToString(sb); + return sb.ToString(); + } + + internal void ToString(StringBuilder sb) + { + int beforeLength = sb.Length; + + // Add the Cookie version if necessary. + if (Version != 0) + { + sb.Append(SpecialAttributeLiteral + VersionAttributeName + EqualsLiteral); // const strings + if (IsQuotedVersion) sb.Append('"'); + sb.Append(m_version.ToString(NumberFormatInfo.InvariantInfo)); + if (IsQuotedVersion) sb.Append('"'); + sb.Append(SeparatorLiteral); + } + + // Add the Cookie Name=Value pair. + sb.Append(Name).Append(EqualsLiteral).Append(Value); + + if (!Plain) + { + // Add the Path if necessary. + if (!m_path_implicit && m_path.Length > 0) + { + sb.Append(SeparatorLiteral + SpecialAttributeLiteral + PathAttributeName + EqualsLiteral); // const strings + sb.Append(m_path); + } + + // Add the Domain if necessary. + if (!m_domain_implicit && m_domain.Length > 0) + { + sb.Append(SeparatorLiteral + SpecialAttributeLiteral + DomainAttributeName + EqualsLiteral); // const strings + if (IsQuotedDomain) sb.Append('"'); + sb.Append(m_domain); + if (IsQuotedDomain) sb.Append('"'); + } + } + + // Add the Port if necessary. + if (!m_port_implicit) + { + sb.Append(SeparatorLiteral + SpecialAttributeLiteral + PortAttributeName); // const strings + if (m_port.Length > 0) + { + sb.Append(EqualsLiteral); + sb.Append(m_port); + } + } + + // Check to see whether the only thing we added was "=", and if so, + // remove it so that we leave the StringBuilder unchanged in contents. + int afterLength = sb.Length; + if (afterLength == (1 + beforeLength) && sb[beforeLength] == '=') { - return string.Empty; + sb.Length = beforeLength; } - return result; } internal string ToServerString()