Optimize huffman encoding static table initialization #45303

rokonec · 2020-11-28T23:52:19Z

Reasoning is in #45297

ghost · 2020-11-28T23:52:24Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Reasoning is in #45297

Author:	rokonec
Assignees:	-
Labels:	`area-System.Net.Http`
Milestone:	-

rokonec · 2020-11-28T23:57:27Z

Although it feels fragile to split it into two array, I feel confident about this changes as unit tests have their own copy of encoding table

runtime/src/libraries/Common/tests/Tests/System/Net/aspnetcore/Http2/HuffmanDecodingTests.cs

Line 363 in 0d5e8d3

TheoryData<int, uint, int> data = new TheoryData<int, uint, int>();

and cover all encoding table items.

src/libraries/Common/src/System/Net/Http/aspnetcore/Http2/Hpack/Huffman.cs

stephentoub

Other than my comments, LGTM. Thanks.

benaadams · 2020-11-29T03:24:47Z

runtime (Libraries Test Run release coreclr windows x86 Release)

System.Text.Encodings.Web.Tests.UnicodeHelpersTests.GetUtf8RepresentationForScalarValue

Error message
System.ArgumentNullException : Value cannot be null. (Parameter 'type')

Stack trace
   at System.Reflection.IntrospectionExtensions.GetTypeInfo(Type type) in /_/src/libraries/System.Private.CoreLib/src/System/Reflection/IntrospectionExtensions.cs:line 11
   at System.Linq.Enumerable.SelectArrayIterator`2.MoveNext() in /_/src/libraries/System.Linq/src/System/Linq/Select.cs:line 186
   at System.Linq.Enumerable.WhereSelectEnumerableIterator`2.MoveNext() in /_/src/libraries/System.Linq/src/System/Linq/Where.cs:line 391
   at System.Linq.Enumerable.Contains[TSource](IEnumerable`1 source, TSource value, IEqualityComparer`1 comparer) in /_/src/libraries/System.Linq/src/System/Linq/Contains.cs:line 23
   at System.Linq.Enumerable.Contains[TSource](IEnumerable`1 source, TSource value) in /_/src/libraries/System.Linq/src/System/Linq/Contains.cs:line 11
   at System.Text.Encodings.Web.Tests.UnicodeHelpersTests.GetUtf8RepresentationForScalarValue() in /_/src/libraries/System.Text.Encodings.Web/tests/UnicodeHelpersTests.cs:line 107

Error is unrelated; but odd as is a null type 🤔

stephentoub · 2020-11-29T11:46:55Z

That stack makes no sense to me; it's pointing to

runtime/src/libraries/System.Text.Encodings.Web/tests/UnicodeHelpersTests.cs

Line 107 in 6072e4d

Assert.Equal(expectedUtf8Bytes, actualUtf8Bytes);

, which doesn't use Select.Where.Contains. @trylek, could this be fallout from the crossgen2 change somehow? This same test has been failing on other PRs this weekend, e.g. https://dev.azure.com/dnceng/public/_build/results?buildId=902244&view=ms.vss-test-web.build-test-results-tab&runId=28813620&paneView=debug&resultId=174751 from @jkotas.

benaadams · 2020-11-29T16:35:15Z

which doesn't use Select.Where.Contains

I assume that's the stack trace for Xunit.Asset.Equal<T>(IEnumerable<T> expected, IEnumerable<T> actual) for List<byte> as Xunit hides its stack frames.

Have been looking at odd release coreclr windows x86 Release null refs in reflection related stacks (this error is one); seeing if I'd broken something in open PR #45121 or whether they are already broken, another is https://dev.azure.com/dnceng/public/_build/results?buildId=902114&view=ms.vss-test-web.build-test-results-tab&runId=28808954&resultId=110325&paneView=debug

System.ComponentModel.Composition.ComposablePartCatalogExportProviderTests.ComponentCatalogResolverGetStaticExport

System.NullReferenceException : Object reference not set to an instance of an object.

at Microsoft.Internal.AttributeServices.IsAttributeDefined[T](ICustomAttributeProvider attributeProvider, Boolean inherit) in /_/src/libraries/System.ComponentModel.Composition/src/Microsoft/Internal/AttributeServices.cs:line 38
   at System.ComponentModel.Composition.AttributedModel.AttributedPartCreationInfo.IsInheritedExport(ICustomAttributeProvider attributedProvider) in /_/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/AttributedModel/AttributedPartCreationInfo.cs:line 423
   at System.ComponentModel.Composition.AttributedModel.AttributedPartCreationInfo.GetInheritedExports(Type type)+MoveNext() in /_/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/AttributedModel/AttributedPartCreationInfo.cs:line 409
   at System.ComponentModel.Composition.AttributedModel.AttributedPartCreationInfo.GetExportDefinitions() in /_/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/AttributedModel/AttributedPartCreationInfo.cs:line 301
   at System.ComponentModel.Composition.AttributedModel.AttributedPartCreationInfo.DiscoverExportsAndImports() in /_/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/AttributedModel/AttributedPartCreationInfo.cs:line 260
   at System.ComponentModel.Composition.AttributedModel.AttributedPartCreationInfo.GetExports() in /_/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/AttributedModel/AttributedPartCreationInfo.cs:line 66
   at System.ComponentModel.Composition.ReflectionModel.ReflectionComposablePartDefinition.get_ExportDefinitionsInternal() in /_/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/ReflectionModel/ReflectionComposablePartDefinition.cs:line 66
   at System.ComponentModel.Composition.ReflectionModel.ReflectionComposablePartDefinition.get_ExportDefinitions() in /_/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/ReflectionModel/ReflectionComposablePartDefinition.cs:line 83
   at System.ComponentModel.Composition.Hosting.TypeCatalog.CreateIndex() in /_/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/Hosting/TypeCatalog.cs:line 322
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode) in /_/src/libraries/System.Private.CoreLib/src/System/Lazy.cs:line 321
   at System.Lazy`1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor) in /_/src/libraries/System.Private.CoreLib/src/System/Lazy.cs:line 345
   at System.Lazy`1.CreateValue() in /_/src/libraries/System.Private.CoreLib/src/System/Lazy.cs:line 425
   at System.Lazy`1.get_Value() in /_/src/libraries/System.Private.CoreLib/src/System/Lazy.cs:line 505
   at System.ComponentModel.Composition.Hosting.TypeCatalog.GetCandidateParts(String contractName) in /_/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/Hosting/TypeCatalog.cs:line 312
   at System.ComponentModel.Composition.Hosting.TypeCatalog.GetCandidateParts(ImportDefinition definition) in /_/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/Hosting/TypeCatalog.cs:line 299
   at System.ComponentModel.Composition.Primitives.ComposablePartCatalog.GetExports(ImportDefinition definition) in /_/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/Primitives/ComposablePartCatalog.cs:line 104
   at System.ComponentModel.Composition.Hosting.AssemblyCatalog.GetExports(ImportDefinition definition) in /_/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/Hosting/AssemblyCatalog.cs:line 427
   at System.ComponentModel.Composition.Hosting.CatalogExportProvider.InternalGetExportsCore(ImportDefinition definition, AtomicComposition atomicComposition) in /_/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/

stephentoub · 2020-11-29T17:30:41Z

I assume that's the stack trace for Xunit.Asset.Equal(IEnumerable expected, IEnumerable actual)

That was my initial thought as well, though I didn't see that anywhere in the assert code (nor is it clear to me why Contains would be used in such an assert), hence my comment.

benaadams · 2020-11-29T18:27:42Z

hence my comment.

Yeah, can't see how it would get there :-/

benaadams · 2020-11-29T18:40:17Z

Sorry @rokonec for this side discussion 😅

@stephentoub a more complete trace from #45310; again on (Libraries Test Run release coreclr windows x86 Release) that looks related (same trace with Xunit but Fatal error. 0xC0000005) https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-45310-merge-f380f19a901d423e93/System.Text.Encodings.Web.Tests/console.31ef414e.log?sv=2019-07-07&se=2020-12-19T16%3A41%3A38Z&sr=c&sp=rl&sig=J6tmdBZJgh2VxjCYoed1fJlnVURu6wUExNd29SkhlgU%3D

C:\h\w\A748094D\w\9B3408E2\e>"C:\h\w\A748094D\p\dotnet.exe" exec --runtimeconfig System.Text.Encodings.Web.Tests.runtimeconfig.json --depsfile System.Text.Encodings.Web.Tests.deps.json xunit.console.dll System.Text.Encodings.Web.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
  Discovering: System.Text.Encodings.Web.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Text.Encodings.Web.Tests (found 181 test cases)
  Starting:    System.Text.Encodings.Web.Tests (parallel test collections = on, max threads = 2)
Fatal error. 0xC0000005
   at System.Runtime.CompilerServices.CastHelpers.IsInstanceOfInterface(Void*, System.Object)
   at System.Reflection.IntrospectionExtensions.GetTypeInfo(System.Type)
   at Xunit.Sdk.AssertEqualityComparer`1+<>c[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].<IsSet>b__10_0(System.Type)
   at System.Linq.Enumerable+SelectArrayIterator`2[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext()
   at System.Linq.Enumerable+WhereSelectEnumerableIterator`2[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext()
   at System.Linq.Enumerable.Contains[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.Collections.Generic.IEnumerable`1<System.__Canon>, System.__Canon, System.Collections.Generic.IEqualityComparer`1<System.__Canon>)
   at System.Linq.Enumerable.Contains[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.Collections.Generic.IEnumerable`1<System.__Canon>, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].IsSet(System.Reflection.TypeInfo)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CheckIfSetsAreEqual(System.__Canon, System.__Canon, System.Reflection.TypeInfo)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.__Canon, System.__Canon)
   at Xunit.Assert.Equal[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.__Canon, System.__Canon, System.Collections.Generic.IEqualityComparer`1<System.__Canon>)
   at Xunit.Assert.Equal[[System.Byte, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.Collections.Generic.IEnumerable`1<Byte>, System.Collections.Generic.IEnumerable`1<Byte>)
   at System.Text.Encodings.Web.Tests.UnicodeHelpersTests.GetUtf8RepresentationForScalarValue()
...

benaadams · 2020-11-29T19:11:02Z

Raised issue for the null ref #45317 as it definitely looks like a thing

rokonec · 2020-11-30T09:25:47Z

@Tratcher can you please review this PR. It is slightly related to previous huffman PR, but considerable simpler and safer changes.

Tratcher · 2020-11-30T17:56:25Z

@Tratcher can you please review this PR. It is slightly related to previous huffman PR, but considerable simpler and safer changes.

Not immediately, I'm dealing with builds for the next two days.

rokonec · 2020-12-03T21:13:13Z

@Tratcher, @halter73 Can any of you please review this PR. This one is really simple.

Tratcher

wait, this is how much better? 😆

rokonec added 2 commits November 29, 2020 00:31

Split into two static arrays

493b9dd

Comment style

3b42db2

rokonec requested a review from a team November 28, 2020 23:52

Dotnet-GitSync-Bot added the area-System.Net.Http label Nov 28, 2020

rokonec mentioned this pull request Nov 28, 2020

HTTP2: Optimize huffman encoding static table initialization #45297

Closed

stephentoub reviewed Nov 29, 2020

View reviewed changes

src/libraries/Common/src/System/Net/Http/aspnetcore/Http2/Hpack/Huffman.cs Show resolved Hide resolved

stephentoub reviewed Nov 29, 2020

View reviewed changes

src/libraries/Common/src/System/Net/Http/aspnetcore/Http2/Hpack/Huffman.cs Outdated Show resolved Hide resolved

stephentoub reviewed Nov 29, 2020

View reviewed changes

src/libraries/Common/src/System/Net/Http/aspnetcore/Http2/Hpack/Huffman.cs Outdated Show resolved Hide resolved

stephentoub approved these changes Nov 29, 2020

View reviewed changes

benaadams mentioned this pull request Nov 29, 2020

Crossgen2-compiled System.Private.CoreLib on Windows x86 causes Null ref in library tests #45317

Closed

Review changes - stephentoub dotnet#1

da44917

This was referenced Nov 30, 2020

Mono System.Text.Json.Tests on Windows timing out #42677

Open

Testing out the new system jaredpar/runfo#71

Open

stephentoub approved these changes Nov 30, 2020

View reviewed changes

Tratcher self-requested a review December 4, 2020 20:19

Tratcher approved these changes Dec 4, 2020

View reviewed changes

rokonec merged commit c2404a9 into dotnet:master Dec 4, 2020

ghost locked as resolved and limited conversation to collaborators Jan 3, 2021

karelz added this to the 6.0.0 milestone Jan 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize huffman encoding static table initialization #45303

Optimize huffman encoding static table initialization #45303

rokonec commented Nov 28, 2020

ghost commented Nov 28, 2020

rokonec commented Nov 28, 2020 •

edited

Loading

stephentoub left a comment

benaadams commented Nov 29, 2020

stephentoub commented Nov 29, 2020 •

edited

Loading

benaadams commented Nov 29, 2020

stephentoub commented Nov 29, 2020 •

edited

Loading

benaadams commented Nov 29, 2020

benaadams commented Nov 29, 2020 •

edited

Loading

benaadams commented Nov 29, 2020

rokonec commented Nov 30, 2020 •

edited

Loading

Tratcher commented Nov 30, 2020

rokonec commented Dec 3, 2020

Tratcher left a comment •

edited

Loading

Optimize huffman encoding static table initialization #45303

Optimize huffman encoding static table initialization #45303

Conversation

rokonec commented Nov 28, 2020

ghost commented Nov 28, 2020

rokonec commented Nov 28, 2020 • edited Loading

stephentoub left a comment

Choose a reason for hiding this comment

benaadams commented Nov 29, 2020

stephentoub commented Nov 29, 2020 • edited Loading

benaadams commented Nov 29, 2020

stephentoub commented Nov 29, 2020 • edited Loading

benaadams commented Nov 29, 2020

benaadams commented Nov 29, 2020 • edited Loading

benaadams commented Nov 29, 2020

rokonec commented Nov 30, 2020 • edited Loading

Tratcher commented Nov 30, 2020

rokonec commented Dec 3, 2020

Tratcher left a comment • edited Loading

Choose a reason for hiding this comment

rokonec commented Nov 28, 2020 •

edited

Loading

stephentoub commented Nov 29, 2020 •

edited

Loading

stephentoub commented Nov 29, 2020 •

edited

Loading

benaadams commented Nov 29, 2020 •

edited

Loading

rokonec commented Nov 30, 2020 •

edited

Loading

Tratcher left a comment •

edited

Loading