Temporarily removing use of ReadOnlySpan indexer in Runtime.Extensions #25326
Conversation
Why is this needed? What breaks without it? I realize the change is intended to only be temporary, but it also adds a gigantic expense to some core methods. I don't like the idea of it being in the code base, even temporarily. |
You can unsafe casts the ReadOnlySpan to Span to address this. |
|
So what is your plan to get this through? Which PR will got first, second, third, ... ? |
The tests in coreclr depend on packages from corefx which won't have the updated ref readonly indexer. So, the tests won't pass in either repo (without some effort to break the dependencoes).
That is a good idea to avoid the performance hit. I will make the change.
|
|
And the plan for UWP NETNative? |
|
|
||
| unsafe | ||
| { | ||
| preamble = new Span<byte>(Unsafe.AsPointer<byte>(ref readonlyPreamble.DangerousGetPinnableReference()), readonlyPreamble.Length); |
There was a problem hiding this comment.
You need to pin the pointer while you are manipulating it.
When step 3 happens (i.e. change to CoreCLR), I would make the change on the .NET Native side as well at the same time. We can then merge the corefx PR (step 4), when we get the coreclr and .NET Native update. Example of PRs that would need to be merged together with the corefx change (from step 4): |
…xtensions (dotnet#25326)" This reverts commit 1d95739.
dotnet/corefx#25326) * Temporarily removing use of Span indexer in Runtime.Extensions * Use Unsafe to cast ReadOnlySpan to Span. * Only add project reference to Unsafe if target group isn't uapaot * Pin the pointer from DangerousGetPinnableReference * Remove unnecessary reference to compilerservices * Remove the reference from the csproj Commit migrated from dotnet/corefx@1d95739
…xtensions (dotnet/corefx#25326)" This reverts commit dotnet/corefx@1d95739. Commit migrated from dotnet/corefx@2e5a18a
Removing the use of the ReadOnlySpan indexer to help the implementation change go through with the least disabling of tests.
This is a temporary workaround that is required to reduce the number of failing tests that would need to be disabled due to the change to ReadOnlySpan indexer to return ref readonly, here: dotnet/coreclr#14727
cc @weshaggard, @stephentoub, @KrzysztofCwalina, @jkotas
This change should be reverted once the change propogates (and the tests enabled): https://github.com/dotnet/coreclr/issues/15089