Skip to content

Reduce Allocation when choosing the Destination#195

Merged
Tratcher merged 9 commits intodotnet:masterfrom
Kahbazi:kahbazi/DestinationArray
Jun 2, 2020
Merged

Reduce Allocation when choosing the Destination#195
Tratcher merged 9 commits intodotnet:masterfrom
Kahbazi:kahbazi/DestinationArray

Conversation

@Kahbazi
Copy link
Copy Markdown
Member

@Kahbazi Kahbazi commented May 22, 2020

Comment thread src/ReverseProxy/Service/RuntimeModel/DestinationInfo.cs Outdated
Comment thread src/ReverseProxy/Service/RuntimeModel/DestinationInfo.cs Outdated
Comment thread src/ReverseProxy/Service/RuntimeModel/DestinationInfo.cs Outdated
Comment thread src/ReverseProxy/Service/RuntimeModel/DestinationInfo.cs
@Tratcher
Copy link
Copy Markdown
Member

Tests are failing with a stack overflow?

   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparerAdapter`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.Object, System.Object)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CheckIfEnumerablesAreEqual(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparerAdapter`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.Object, System.Object)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CheckIfEnumerablesAreEqual(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparerAdapter`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.Object, System.Object)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CheckIfEnumerablesAreEqual(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.__Canon, System.__Canon)
   at Xunit.Assert.Equal[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.__Canon, System.__Canon, System.Collections.Generic.IEqualityComparer`1<System.__Canon>)
   at Xunit.Assert.Equal[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.__Canon, System.__Canon)
   at Microsoft.ReverseProxy.Service.Proxy.Tests.LoadBalancerTests.PickDestination_SingleDestinations_ShortCircuit()

https://github.com/microsoft/reverse-proxy/blob/86f9c12a78a1cd79298d3e22fd92f524be052e0f/test/ReverseProxy.Tests/Service/Proxy/LoadBalancerTests.cs#L47-L49

XUnit can't seem to figure out if it should compare these as objects or lists. I don't think there's anything wrong with your implementation, this might just be that the complier is defaulting var result to IReadOnlyList for lack of a more specific restraint. Try updating the test?

@halter73
Copy link
Copy Markdown
Member

Now DynamicState_WithHealthChecks_HonorsHealthState is failing with a stack overflow.

It seems XUnit's Assert.Equal really doesn't play nice with lists that return themselves as a list item. It just keeps recursively enumerating what it sees as an infinitely nested list. Anything that tries to call Assert.Equal on a DestinationInfo or anything containing a DestinationInfo will fail this way.

Stack overflow.
   at System.Linq.Enumerable.Where[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.Collections.Generic.IEnumerable`1<System.__Canon>, System.Func`2<System.__Canon,Boolean>)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].IsSet(System.Reflection.TypeInfo)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.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=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparerAdapter`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.Object, System.Object)
...
   at Xunit.Sdk.AssertEqualityComparerAdapter`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.Object, System.Object)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CheckIfEnumerablesAreEqual(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparerAdapter`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.Object, System.Object)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CheckIfEnumerablesAreEqual(System.__Canon, System.__Canon)
   at Xunit.Sdk.AssertEqualityComparer`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Equals(System.__Canon, System.__Canon)
   at Xunit.Assert.Equal[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.__Canon, System.__Canon, System.Collections.Generic.IEqualityComparer`1<System.__Canon>)
   at Xunit.Assert.Equal[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.__Canon, System.__Canon)
   at Microsoft.ReverseProxy.RuntimeModel.Tests.BackendInfoTests.DynamicState_WithHealthChecks_HonorsHealthState()

@Kahbazi If you click the "View more details on Azure Pipelines" link at the bottom of this PR's "Checks" tab, you should be able to see the test failure details by downloading the "2 published" artifacts. We still need to fix things so test failures are properly reported. @Tratcher Is there already an issue tracking improving the test failure output in PR checks?

image

It is nice that stack overflows finally show stack traces though! dotnet/runtime#31956

@Tratcher
Copy link
Copy Markdown
Member

Is there already an issue tracking improving the test failure output in PR checks?

I had assumed the test reporting was bad in this case because of the StackOverflow. I can try it with a more typical error in another PR.

@Tratcher
Copy link
Copy Markdown
Member

Yeah, normal test failures work. StackOveflow is special.
https://dev.azure.com/dnceng/public/_build/results?buildId=664478&view=ms.vss-test-web.build-test-results-tab&runId=20601422&resultId=100187&paneView=debug

@Kahbazi Kahbazi requested a review from Tratcher May 31, 2020 17:24
@Tratcher Tratcher requested a review from alnikola May 31, 2020 19:16
@Tratcher Tratcher added this to the 1.0.0-preview2 milestone Jun 2, 2020
@Tratcher Tratcher self-assigned this Jun 2, 2020
@Tratcher Tratcher merged commit 10a355d into dotnet:master Jun 2, 2020
@Tratcher
Copy link
Copy Markdown
Member

Tratcher commented Jun 2, 2020

Thanks

@Kahbazi Kahbazi deleted the kahbazi/DestinationArray branch June 3, 2020 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants