Skip to content

Remove dependency on System.Memory/System.Buffers. - #4991

Merged
manfred-brands merged 6 commits into
mainfrom
Issue4990_FileLoadException
Jun 4, 2025
Merged

manfred-brands merged 6 commits into
mainfrom
Issue4990_FileLoadException

Conversation

@manfred-brands

Copy link
Copy Markdown
Member

Fixes #4990

Added Array.SequenceEqual
Added LocalPool

@OsirisTerje OsirisTerje left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@stevenaw stevenaw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too thanks @manfred-brands !

Incidentally, I also just hit the problem this seeks to fix an hour ago at work so this is a very timely fix for me in that regard.

We needed to remove the nullable attributes because of the InternalsVisibleTo
Resulting in two definitions competing.

Removed stackalloc as it isn't used and there is no Read(Span) method in .NET Framework
@manfred-brands
manfred-brands force-pushed the Issue4990_FileLoadException branch from a4b358d to e34a50b Compare June 3, 2025 05:50
@manfred-brands

Copy link
Copy Markdown
Member Author

I looked a but more in the SequenceEquals and the implementation on .NET6+ calls the Span overloads which explains that both have the same performance. Unfortunately the method on .NET Framework is VERY inefficient.

| Method             | Runtime              | Size  | Mean       | Error     | Ratio |
|------------------- |--------------------- |------ |-----------:|----------:|------:|
| Original           | .NET 8.0             | 32768 |   9.413 us | 0.0950 us |  1.00 |
| Vectorized         | .NET 8.0             | 32768 |   1.535 us | 0.0306 us |  0.16 |
| ArraySequenceEqual | .NET 8.0             | 32768 |   1.578 us | 0.0087 us |  0.17 |
|                    |                      |       |            |           |       |
| Original           | .NET Framework 4.6.2 | 32768 |  11.803 us | 0.2333 us |  1.00 |
| Vectorized         | .NET Framework 4.6.2 | 32768 |   1.538 us | 0.0149 us |  0.13 |
| ArraySequenceEqual | .NET Framework 4.6.2 | 32768 | 307.529 us | 1.0167 us | 26.07 |

I therefore updated the code to comment out the call to SequenceEqual for .NET Framework.
This means we either have a dependency on System.Memory/Buffers or for .NET Framework we have the original performance.

I don't know actual usages, but I don't expect StreamsComparer to be used so much.
Also anyone still using .NET Framework knows it is much slower than later versions of .NET for everything.

@manfred-brands

Copy link
Copy Markdown
Member Author

@nunit/framework-team I am at a loss why nunitlite-runner fails on the .net6.0 build.
It claims it cannot load System.Collections.Immutable version 8.0.0

Both dotnet test and running the test in VS work fine. The correct version of System.Collections.Immutable is in the directory.

I needed to up the version to 8.0.0 for BenchmarkDotnet.
NUnit only uses System.Collections.Immutable for test projects.
We could move the Benchmark project to a separate assembly.
However, I would like to know why it doesn't work.

@manfred-brands

Copy link
Copy Markdown
Member Author

I added a commit to revert the multi-target running of the benchmarks including changing back to version 6.0.0 of System.Collections.Immutable in order to get a green build.

@OsirisTerje

Copy link
Copy Markdown
Member

I'm not worried about a performance degradation for .net framework. So just let that be imho.

The Immutable 6.0 issue: It only affects the benchmark project, right? If so, if w only run that on net8, I would be fine with that.

@manfred-brands

Copy link
Copy Markdown
Member Author

The Immutable 6.0 issue: It only affects the benchmark project, right?

No, it is nunit.framework.tests that fails when it determines immutable test cases.

@stevenaw

stevenaw commented Jun 3, 2025

Copy link
Copy Markdown
Member

I've hit the immutable issue in the tests in the past as well. At the time I had opted to go back to V6 after a bit of debugging without getting anywhere. I'm unsure the root cause myself.

Agreed too about netfx performance. Generally speaking, users on that runtime should already be aware that they aren't getting all the performance they could be

Until we have resolved the nunitlite-runner issues with System.Collections.Immutable 8.0
@manfred-brands
manfred-brands force-pushed the Issue4990_FileLoadException branch from e7e8141 to 286764c Compare June 4, 2025 07:10
@manfred-brands
manfred-brands merged commit f617afb into main Jun 4, 2025
@manfred-brands
manfred-brands deleted the Issue4990_FileLoadException branch June 4, 2025 07:18
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.

FileLoadException for NUnit 4.3.2: System.Buffers 4.0.4 missing

3 participants