From eee05d40f2fb07c946961748a431f05dffafbf4b Mon Sep 17 00:00:00 2001 From: pedrobsaila Date: Wed, 29 May 2024 23:11:16 +0200 Subject: [PATCH 1/4] TryAddWithoutValidation for multiple values could be more efficient --- .../System/Net/Http/Headers/HttpHeaders.cs | 66 +++++++++++++++++++ .../UnitTests/Headers/HttpHeadersTest.cs | 60 +++++++++++++++++ 2 files changed, 126 insertions(+) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index 56015f488aaae2..f3badea85eb252 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -169,6 +169,15 @@ internal bool TryAddWithoutValidation(HeaderDescriptor descriptor, IEnumerable valuesList) + { + return SpecializedTryAddWithoutValidation(descriptor, null, valuesList); + } + using IEnumerator enumerator = values.GetEnumerator(); if (enumerator.MoveNext()) { @@ -196,6 +205,63 @@ internal bool TryAddWithoutValidation(HeaderDescriptor descriptor, IEnumerable? valuesList) + { + Debug.Assert(valuesArray != null || valuesList != null); + int count = valuesArray != null ? valuesArray.Length : valuesList!.Count; + + if (count == 0) + { + return true; + } + + ref object? storeValueRef = ref GetValueRefOrAddDefault(descriptor); + object? storeValue = storeValueRef; + + if (storeValue is not null || count > 1) + { + if (storeValue is not HeaderStoreItemInfo info) + { + storeValueRef = info = new HeaderStoreItemInfo { RawValue = storeValue }; + } + + object? rawValue = info.RawValue; + if (rawValue is not List rawValues) + { + info.RawValue = rawValues = new List(); + if (rawValue is not null) + { + rawValues.EnsureCapacity(count + 1); + rawValues.Add((string)rawValue); + } + } + + rawValues.EnsureCapacity(count); + + if (valuesArray != null) + { + for (int i = 0; i < valuesArray.Length; i++) + { + rawValues.Add(valuesArray[i] ?? string.Empty); + } + } + else + { + for (int i = 0; i < valuesList!.Count; i++) + { + rawValues.Add(valuesList[i] ?? string.Empty); + } + } + } + else + { + Debug.Assert(count == 1 && storeValue is null); + storeValueRef = (valuesArray != null ? valuesArray[0] : valuesList![0]) ?? string.Empty; + } + + return true; + } + public IEnumerable GetValues(string name) => GetValues(GetHeaderDescriptor(name)); internal IEnumerable GetValues(HeaderDescriptor descriptor) diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs index 9def1da0e76bfd..9cf6a26fdf6c43 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs @@ -2544,6 +2544,66 @@ public void TryGetValues_InvalidValuesContainingNewLines_ShouldNotRemoveInvalidV Assert.Equal(value, values.Single()); } + [Fact] + public void TryAddWithoutValidation_OneValidValueHeader_UseSpecialListImplementation() + { + const string Name = "customHeader1"; + const string Value = "Value1"; + + var response = new HttpResponseMessage(); + Assert.True(response.Headers.TryAddWithoutValidation(Name, new List { Value })); + + Assert.True(response.Headers.Contains(Name)); + + Assert.True(response.Headers.TryGetValues(Name, out IEnumerable values)); + Assert.Equal(Value, values.Single()); + } + + [Fact] + public void TryAddWithoutValidation_ThreeValidValueHeader_UseSpecialListImplementation() + { + const string Name = "customHeader1"; + List expectedValues = [ "Value1", "Value2", "Value3" ]; + + var response = new HttpResponseMessage(); + Assert.True(response.Headers.TryAddWithoutValidation(Name, expectedValues)); + + Assert.True(response.Headers.Contains(Name)); + + Assert.True(response.Headers.TryGetValues(Name, out IEnumerable values)); + Assert.True(expectedValues.SequenceEqual(values)); + } + + [Fact] + public void TryAddWithoutValidation_OneValidValueHeader_UseSpecialArrayImplementation() + { + const string Name = "customHeader1"; + const string Value = "Value1"; + + var response = new HttpResponseMessage(); + Assert.True(response.Headers.TryAddWithoutValidation(Name, new [] { Value })); + + Assert.True(response.Headers.Contains(Name)); + + Assert.True(response.Headers.TryGetValues(Name, out IEnumerable values)); + Assert.Equal(Value, values.Single()); + } + + [Fact] + public void TryAddWithoutValidation_ThreeValidValueHeader_UseSpecialArrayImplementation() + { + const string Name = "customHeader1"; + string[] expectedValues = ["Value1", "Value2", "Value3"]; + + var response = new HttpResponseMessage(); + Assert.True(response.Headers.TryAddWithoutValidation(Name, expectedValues)); + + Assert.True(response.Headers.Contains(Name)); + + Assert.True(response.Headers.TryGetValues(Name, out IEnumerable values)); + Assert.True(expectedValues.SequenceEqual(values)); + } + public static IEnumerable NumberOfHeadersUpToArrayThreshold_AddNonValidated_EnumerateNonValidated() { for (int i = 0; i <= HttpHeaders.ArrayThreshold; i++) From 7fde1c12b6f9178a3af84711b7839f41806d0083 Mon Sep 17 00:00:00 2001 From: pedrobsaila Date: Fri, 31 May 2024 17:46:48 +0200 Subject: [PATCH 2/4] fix remarks --- .../System/Net/Http/Headers/HttpHeaders.cs | 108 ++++++++++-------- .../UnitTests/Headers/HttpHeadersTest.cs | 30 +++++ 2 files changed, 91 insertions(+), 47 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index f3badea85eb252..b302ee3eed0ef6 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -168,57 +168,28 @@ public bool TryAddWithoutValidation(string name, IEnumerable values) => internal bool TryAddWithoutValidation(HeaderDescriptor descriptor, IEnumerable values) { ArgumentNullException.ThrowIfNull(values); + string?[]? valuesArray = values as string?[]; + IList? valuesList = values as IList; - if (values is string?[] valuesArray) - { - return SpecializedTryAddWithoutValidation(descriptor, valuesArray, null); - } - else if (values is IList valuesList) - { - return SpecializedTryAddWithoutValidation(descriptor, null, valuesList); - } - - using IEnumerator enumerator = values.GetEnumerator(); - if (enumerator.MoveNext()) - { - TryAddWithoutValidation(descriptor, enumerator.Current); - if (enumerator.MoveNext()) - { - ref object? storeValueRef = ref GetValueRefOrAddDefault(descriptor); - Debug.Assert(storeValueRef is not null); - - object value = storeValueRef; - if (value is not HeaderStoreItemInfo info) - { - Debug.Assert(value is string); - storeValueRef = info = new HeaderStoreItemInfo { RawValue = value }; - } - - do - { - AddRawValue(info, enumerator.Current ?? string.Empty); - } - while (enumerator.MoveNext()); - } - } - - return true; - } - - private bool SpecializedTryAddWithoutValidation(HeaderDescriptor descriptor, string?[]? valuesArray, IList? valuesList) - { - Debug.Assert(valuesArray != null || valuesList != null); - int count = valuesArray != null ? valuesArray.Length : valuesList!.Count; + // the count is null when values is not a array nor IList + int? count = valuesArray != null ? valuesArray.Length : valuesList?.Count; if (count == 0) { return true; } + // read the store of header values + // The header store contain a single raw string value when header values are single. + // The header store contain HeaderStoreItemInfo that wraps around a List with header values when they are more than once + ref object? storeValueRef = ref GetValueRefOrAddDefault(descriptor); object? storeValue = storeValueRef; - if (storeValue is not null || count > 1) + // check whether the header store contains already a value + // or header values count is more than one + // => allocate a HeaderStoreItemInfo on store that wraps around a List of header values + if (storeValue is not null || (count.HasValue && count.Value > 1)) { if (storeValue is not HeaderStoreItemInfo info) { @@ -229,14 +200,22 @@ private bool SpecializedTryAddWithoutValidation(HeaderDescriptor descriptor, str if (rawValue is not List rawValues) { info.RawValue = rawValues = new List(); - if (rawValue is not null) + + if (rawValue != null) { - rawValues.EnsureCapacity(count + 1); + if (count.HasValue) + { + rawValues.EnsureCapacity(count.Value + 1); + } + rawValues.Add((string)rawValue); } } - rawValues.EnsureCapacity(count); + if (count.HasValue) + { + rawValues.EnsureCapacity(count.Value); + } if (valuesArray != null) { @@ -245,18 +224,53 @@ private bool SpecializedTryAddWithoutValidation(HeaderDescriptor descriptor, str rawValues.Add(valuesArray[i] ?? string.Empty); } } - else + else if (valuesList != null) { for (int i = 0; i < valuesList!.Count; i++) { rawValues.Add(valuesList[i] ?? string.Empty); } } + else + { + foreach (string? value in values) + { + rawValues.Add(value ?? string.Empty); + } + } + } + else if (count == 1) + { + // when header values are single the store contains just the header value + storeValueRef = valuesArray != null ? valuesArray[0] : (valuesList != null ? valuesList![0] : null); } else { - Debug.Assert(count == 1 && storeValue is null); - storeValueRef = (valuesArray != null ? valuesArray[0] : valuesList![0]) ?? string.Empty; + // handles the case when header values count is unknown because the values are abstracted by IEnumerable + foreach (string? value in values) + { + if (storeValueRef is null) + { + storeValueRef = value; + } + else + { + if (storeValueRef is not HeaderStoreItemInfo info) + { + storeValueRef = info = new HeaderStoreItemInfo { RawValue = storeValueRef }; + } + + object? rawValue = info.RawValue; + + if (rawValue is not List rawValues) + { + info.RawValue = rawValues = new List(2); + rawValues.Add(rawValue == null ? string.Empty : (string)rawValue); + } + + rawValues.Add(value ?? string.Empty); + } + } } return true; diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs index 9cf6a26fdf6c43..621bf52c60a9c0 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs @@ -2604,6 +2604,36 @@ public void TryAddWithoutValidation_ThreeValidValueHeader_UseSpecialArrayImpleme Assert.True(expectedValues.SequenceEqual(values)); } + [Fact] + public void TryAddWithoutValidation_OneValidValueHeader_UseGenericImplementation() + { + const string Name = "customHeader1"; + const string Value = "Value1"; + + var response = new HttpResponseMessage(); + Assert.True(response.Headers.TryAddWithoutValidation(Name, new HashSet { Value })); + + Assert.True(response.Headers.Contains(Name)); + + Assert.True(response.Headers.TryGetValues(Name, out IEnumerable values)); + Assert.Equal(Value, values.Single()); + } + + [Fact] + public void TryAddWithoutValidation_ThreeValidValueHeader_UseGenericImplementation() + { + const string Name = "customHeader1"; + List expectedValues = ["Value1", "Value2", "Value3"]; + + var response = new HttpResponseMessage(); + Assert.True(response.Headers.TryAddWithoutValidation(Name, new HashSet(expectedValues))); + + Assert.True(response.Headers.Contains(Name)); + + Assert.True(response.Headers.TryGetValues(Name, out IEnumerable values)); + Assert.True(expectedValues.SequenceEqual(values)); + } + public static IEnumerable NumberOfHeadersUpToArrayThreshold_AddNonValidated_EnumerateNonValidated() { for (int i = 0; i <= HttpHeaders.ArrayThreshold; i++) From dc32c5e34da9f2497a7b28a2f6a5a9e2641fd2f4 Mon Sep 17 00:00:00 2001 From: pedrobsaila Date: Sat, 1 Jun 2024 01:41:15 +0200 Subject: [PATCH 3/4] keep only ilist special implementation --- .../System/Net/Http/Headers/HttpHeaders.cs | 104 +++++------------- .../UnitTests/Headers/HttpHeadersTest.cs | 30 ----- 2 files changed, 27 insertions(+), 107 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index b302ee3eed0ef6..f797f33f03917b 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -168,108 +168,58 @@ public bool TryAddWithoutValidation(string name, IEnumerable values) => internal bool TryAddWithoutValidation(HeaderDescriptor descriptor, IEnumerable values) { ArgumentNullException.ThrowIfNull(values); - string?[]? valuesArray = values as string?[]; - IList? valuesList = values as IList; - // the count is null when values is not a array nor IList - int? count = valuesArray != null ? valuesArray.Length : valuesList?.Count; - - if (count == 0) + if (values is IList valuesList && valuesList.Count > 0) { - return true; - } + int count = valuesList.Count; - // read the store of header values - // The header store contain a single raw string value when header values are single. - // The header store contain HeaderStoreItemInfo that wraps around a List with header values when they are more than once - - ref object? storeValueRef = ref GetValueRefOrAddDefault(descriptor); - object? storeValue = storeValueRef; + // reads the store of header values + // The header store contain a single raw string value when header values are single. + // The header store contain HeaderStoreItemInfo that wraps around a List with header values when they are more than once - // check whether the header store contains already a value - // or header values count is more than one - // => allocate a HeaderStoreItemInfo on store that wraps around a List of header values - if (storeValue is not null || (count.HasValue && count.Value > 1)) - { - if (storeValue is not HeaderStoreItemInfo info) - { - storeValueRef = info = new HeaderStoreItemInfo { RawValue = storeValue }; - } + ref object? storeValueRef = ref GetValueRefOrAddDefault(descriptor); + object? storeValue = storeValueRef; - object? rawValue = info.RawValue; - if (rawValue is not List rawValues) + // check whether the header store contains already a value + // or header values count is more than one + // => allocate a HeaderStoreItemInfo on store that wraps around a List of header values + if (storeValue is not null || count > 1) { - info.RawValue = rawValues = new List(); + if (storeValue is not HeaderStoreItemInfo info) + { + storeValueRef = info = new HeaderStoreItemInfo { RawValue = storeValue }; + } - if (rawValue != null) + object? rawValue = info.RawValue; + if (rawValue is not List rawValues) { - if (count.HasValue) + info.RawValue = rawValues = new List(); + + if (rawValue != null) { - rawValues.EnsureCapacity(count.Value + 1); + rawValues.EnsureCapacity(count + 1); + rawValues.Add((string)rawValue); } - - rawValues.Add((string)rawValue); } - } - if (count.HasValue) - { - rawValues.EnsureCapacity(count.Value); - } + rawValues.EnsureCapacity(count); - if (valuesArray != null) - { - for (int i = 0; i < valuesArray.Length; i++) - { - rawValues.Add(valuesArray[i] ?? string.Empty); - } - } - else if (valuesList != null) - { - for (int i = 0; i < valuesList!.Count; i++) + for (int i = 0; i < count; i++) { rawValues.Add(valuesList[i] ?? string.Empty); } } else { - foreach (string? value in values) - { - rawValues.Add(value ?? string.Empty); - } + // when header values are single the store contains just the header value + storeValueRef = valuesList[0]; } } - else if (count == 1) - { - // when header values are single the store contains just the header value - storeValueRef = valuesArray != null ? valuesArray[0] : (valuesList != null ? valuesList![0] : null); - } else { - // handles the case when header values count is unknown because the values are abstracted by IEnumerable foreach (string? value in values) { - if (storeValueRef is null) - { - storeValueRef = value; - } - else - { - if (storeValueRef is not HeaderStoreItemInfo info) - { - storeValueRef = info = new HeaderStoreItemInfo { RawValue = storeValueRef }; - } - - object? rawValue = info.RawValue; - - if (rawValue is not List rawValues) - { - info.RawValue = rawValues = new List(2); - rawValues.Add(rawValue == null ? string.Empty : (string)rawValue); - } - - rawValues.Add(value ?? string.Empty); - } + TryAddWithoutValidation(descriptor, value ?? string.Empty); } } diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs index 621bf52c60a9c0..7f87014fb5bbb1 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs @@ -2574,36 +2574,6 @@ public void TryAddWithoutValidation_ThreeValidValueHeader_UseSpecialListImplemen Assert.True(expectedValues.SequenceEqual(values)); } - [Fact] - public void TryAddWithoutValidation_OneValidValueHeader_UseSpecialArrayImplementation() - { - const string Name = "customHeader1"; - const string Value = "Value1"; - - var response = new HttpResponseMessage(); - Assert.True(response.Headers.TryAddWithoutValidation(Name, new [] { Value })); - - Assert.True(response.Headers.Contains(Name)); - - Assert.True(response.Headers.TryGetValues(Name, out IEnumerable values)); - Assert.Equal(Value, values.Single()); - } - - [Fact] - public void TryAddWithoutValidation_ThreeValidValueHeader_UseSpecialArrayImplementation() - { - const string Name = "customHeader1"; - string[] expectedValues = ["Value1", "Value2", "Value3"]; - - var response = new HttpResponseMessage(); - Assert.True(response.Headers.TryAddWithoutValidation(Name, expectedValues)); - - Assert.True(response.Headers.Contains(Name)); - - Assert.True(response.Headers.TryGetValues(Name, out IEnumerable values)); - Assert.True(expectedValues.SequenceEqual(values)); - } - [Fact] public void TryAddWithoutValidation_OneValidValueHeader_UseGenericImplementation() { From 949cd7c8b7a51f8fab9311c38b5929eae38e8ca7 Mon Sep 17 00:00:00 2001 From: pedrobsaila Date: Mon, 3 Jun 2024 23:09:42 +0200 Subject: [PATCH 4/4] fix remarks 2 --- .../System/Net/Http/Headers/HttpHeaders.cs | 63 ++++++++++--------- 1 file changed, 32 insertions(+), 31 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index f797f33f03917b..f22f2d09cc312c 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -169,51 +169,52 @@ internal bool TryAddWithoutValidation(HeaderDescriptor descriptor, IEnumerable valuesList && valuesList.Count > 0) + if (values is IList valuesList) { int count = valuesList.Count; - // reads the store of header values - // The header store contain a single raw string value when header values are single. - // The header store contain HeaderStoreItemInfo that wraps around a List with header values when they are more than once - - ref object? storeValueRef = ref GetValueRefOrAddDefault(descriptor); - object? storeValue = storeValueRef; - - // check whether the header store contains already a value - // or header values count is more than one - // => allocate a HeaderStoreItemInfo on store that wraps around a List of header values - if (storeValue is not null || count > 1) + if (count > 0) { - if (storeValue is not HeaderStoreItemInfo info) - { - storeValueRef = info = new HeaderStoreItemInfo { RawValue = storeValue }; - } + // The store value is either a string (a single unparsed value) or a HeaderStoreItemInfo. + // The RawValue on HeaderStoreItemInfo can likewise be either a single string or a List. + + ref object? storeValueRef = ref GetValueRefOrAddDefault(descriptor); + object? storeValue = storeValueRef; - object? rawValue = info.RawValue; - if (rawValue is not List rawValues) + // If the storeValue was already set or we're adding more than 1 value, + // we'll have to store the values in a List on HeaderStoreItemInfo. + if (storeValue is not null || count > 1) { - info.RawValue = rawValues = new List(); + if (storeValue is not HeaderStoreItemInfo info) + { + storeValueRef = info = new HeaderStoreItemInfo { RawValue = storeValue }; + } - if (rawValue != null) + object? rawValue = info.RawValue; + if (rawValue is not List rawValues) { - rawValues.EnsureCapacity(count + 1); - rawValues.Add((string)rawValue); + info.RawValue = rawValues = new List(); + + if (rawValue != null) + { + rawValues.EnsureCapacity(count + 1); + rawValues.Add((string)rawValue); + } } - } - rawValues.EnsureCapacity(count); + rawValues.EnsureCapacity(rawValues.Count + count); - for (int i = 0; i < count; i++) + for (int i = 0; i < count; i++) + { + rawValues.Add(valuesList[i] ?? string.Empty); + } + } + else { - rawValues.Add(valuesList[i] ?? string.Empty); + // We're adding a single value to a new header entry. We can store the unparsed value as-is. + storeValueRef = valuesList[0] ?? string.Empty; } } - else - { - // when header values are single the store contains just the header value - storeValueRef = valuesList[0]; - } } else {