Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use AssertExtensions.SequenceEqual in a few places #52054

Merged
merged 1 commit into from
May 3, 2021

Conversation

geoffkizer
Copy link
Contributor

@geoffkizer geoffkizer commented Apr 29, 2021

Related to #51480

Use AssertExtensions.SequenceEqual in a few places in the System.Net.Sockets tests.
Also some minor style/spelling cleanup.

@antonfirsov

@ghost
Copy link

ghost commented Apr 29, 2021

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

Issue Details

Related to #51480

Use AssertExtensions.SequenceEqual in a few places in the tests.
Also some minor style/spelling cleanup.

@antonfirsov

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Sockets

Milestone: -

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wfurt
Copy link
Member

wfurt commented Apr 29, 2021

The Assert.Equal already compares the sequence e.g. the bytes, right? Is the goal to make it more visible?

@geoffkizer
Copy link
Contributor Author

Assert.Equal does an IEnumerable-based comparison. The performance of this is terrible for large buffers. So terrible that it was dominating the total time of test runs in the stream conformance tests.

AssertExtensions.SequenceEqual does a span-based comparison, which is much much faster. Like 20x or more.

AssertExtensions.SequenceEqual also has some logic around reporting differences that's kind of nice, but that's secondary to the performance issue.

We should always use AssertExtensions.SequenceEqual for comparing buffers. Assert.Equal is fine for non-buffers.

@geoffkizer
Copy link
Contributor Author

@dotnet/dnceng Getting errors from Azure DevOps, is this a known issue?

@ilyas1974
Copy link

Yes. We ran into a problem with the AzDO DB. We are tracking this in https://github.com/dotnet/core-eng/issues/12969

@wfurt
Copy link
Member

wfurt commented Apr 29, 2021

that make sense.

@stephentoub
Copy link
Member

The performance of this is terrible for large buffers.

This should no longer be the case with .NET 6. Are you still seeing that?

@stephentoub
Copy link
Member

Scratch that, the Assert.Equal implementation looks to be different than i remember it. I could have sworn it was using Enumerable.SequenceEqual, which has addressed this problem. But a quick code inspection suggests it may not be.

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@geoffkizer geoffkizer merged commit 16db832 into dotnet:main May 3, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants