Revert "Improve Span.Reverse fast path performance"

[stephentoub]

Reverts #70944
Fixes #78604
cc: @jkotas, @adamsitnik

[ghost]

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

Issue Details

---

Reverts #70944
Fixes #78604
cc: @jkotas, @adamsitnik

Author:	stephentoub
Assignees:	-
Labels:	area-System.Memory
Milestone:	-

[jkotas]

Are you able to reproduce the regression as well? (I just want to make sure this is not something that only reproduces for me before we revert it.)

[stephentoub]

Are you able to reproduce the regression as well?

Yes.

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System;
using System.Linq;

public partial class Program
{
    static void Main(string[] args) => BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);

    private byte[] _values = new byte[33];

    [Benchmark]
    public void Reverse() => _values.AsSpan().Reverse();
}

dotnet run -c Release -f net7.0 --filter ** --runtimes net7.0 --corerun d:\coreclrtest\main\corerun.exe

Method	Job	Toolchain	Mean	Error	StdDev	Ratio
Reverse	Job-JJOFHY	CoreRun	4.125 ns	0.0098 ns	0.0087 ns	1.39
Reverse	Job-JFSVLH	net7.0	2.969 ns	0.0166 ns	0.0139 ns	1.00

And then after reverting:

Method	Job	Toolchain	Mean	Error	StdDev	Ratio
Reverse	Job-BLQOMH	CoreRun	2.951 ns	0.0319 ns	0.0266 ns	0.99
Reverse	Job-DNDYRC	net7.0	2.984 ns	0.0253 ns	0.0225 ns	1.00

[adamsitnik]

@jkotas thank you for reporting the regression.

@stephentoub @jkotas may I ask why you have decided to revert #70944 so quickly? I am not happy about this for multiple reasons:

• Reversing bytes is not on hot path and we are not close to the release, so we don't need hurry.
• As an owner of System.Memory I had no chance to react to the issue, neither the PR as it was opened on Saturday, and I don't work on weekends.
• It has caused a merge conflict in Unroll SpanHelpers.Reverse() for Vector128 #76076 which I've just solved on Friday to unblock @SwapnilGaikwad.

[stephentoub]

#70944 (comment)

It's the same thing we'd do if a PR introduced a repeatable CI failure, a behavioral regression, etc: back out the change as quickly as possible to get things back to a known state, and then enable the relevant parties to try again without time pressure.

[adamsitnik]

back out the change as quickly as possible to get things back to a known state

I understand such approach when the CI is red, or we are regressing an IMPORTANT scenario.

From my perspective you have caused a lot more work for me:

• I'll need to review a large nontrivial PR again.
• I'll need to solve the merge conflict again.
• I'll need to wait for the CI to complete at least twice.
• Unroll SpanHelpers.Reverse() for Vector128 #76076 is blocked again, I'll need to reach out to the contributor and explain the new current situation.

If I was given the time to react, I would just send a small PR with a fix for the bytes code path. It would take me half an hour including running the benchmarks.

[stephentoub]

Then you can revert the revert locally, without re-reviewing it, make the change as a commit on top of that, it won't take you any more time, it won't conflict, etc., and in the meantime we haven't incurred a perf regression that other things (like the very PR you're worried about conflicting) then layer on top of, compounding the problems and making it harder to understand / investigate, causing noise and work for all the folks that manage interactions with the performance reporting system, etc.

[adamsitnik]

I could do that, but the contributor who contributed the PR has already started working on the fix: #70944 (comment)

[stephentoub]

the contributor who contributed the PR has already started working on the fix

That was a day ago, and you stated it would take you "half an hour including running the benchmarks". If you really believe you would have a new PR up in 30 minutes from now, and it'll overall save you a lot of time, I suggest you do so, with thanks to @yesmey for their efforts. If you don't want to do that, it still shouldn't take you a lot of time, as you can diff the previous commit with the new one and only review the difference, resulting in little additional work, and still minimal impact on the other PR you're concerned about.

[yesmey]

Didn't have time yesterday and I just got back from work :) Running the benchmarks now

[adamsitnik]

causing noise and work for all the folks that manage interactions with the performance reporting system

There would be no noise as we were missing coverage for this code path. PTAL at dotnet/performance#2748

[stephentoub]

There would be no noise as we were missing coverage for this code path.

There are multiple uses of Array.Reverse and MemoryExtensions.Reverse throughout the codebase, including for bytes.