Remove a few instances of unsafe code #116111
Conversation
github-actions
bot
added
the
needs-area-label
EgorBo
removed
the
needs-area-label
src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Memory/src/System/Buffers/SequenceReaderExtensions.Binary.cs
Outdated
Show resolved
Hide resolved
|
|
||
| return (int)(hash1 + (hash2 * 1566083941)); | ||
| } | ||
| return GetNonRandomizedHashCode(new ReadOnlySpan<char>(ref _firstChar, _stringLength)); |
There was a problem hiding this comment.
Does this negatively impact perf?
The implementation as it existed was benefiting from strings being null terminated. The span-based overload can't rely on that and pays a tad for it.
See large comment above.
There was a problem hiding this comment.
At the same time the Span one special cased 1,2,3 length, it was an improvement last time I ran benchmark, I'll do again
This comment was marked as resolved.
This comment was marked as resolved.
| @@ -39,16 +39,15 @@ private static unsafe bool TryReadMultisegment<T>(ref SequenceReader<byte> reade | |||
| Debug.Assert(reader.UnreadSpan.Length < sizeof(T)); | |||
|
|
|||
| // Not enough data in the current segment, try to peek for the data we need. | |||
| T buffer = default; | |||
| Span<byte> tempSpan = new Span<byte>(&buffer, sizeof(T)); | |||
| Span<byte> tempSpan = stackalloc byte[sizeof(T)]; | |||
There was a problem hiding this comment.
This seems like a regression?
SharpLab
There was a problem hiding this comment.
I am sure whoever wrote the initial code was trying to avoid the stack probe/GS cookie with this workaround. I think this sets a bad example and we should never accept code like this anymore, but does it mean we're going to leave all the existing places as is?
To be honest, at this point I suspect we're going to focus on unsafe only in places nobody use on the hot path because so far in most places regressions are not avoidable like the one with null terminator, etc.
There was a problem hiding this comment.
I don't see a reason why a stack probe is any more necessary in M1 than in M2, or in M3 in SharpLab. The JIT should be able to avoid them with const sized stackallocs into spans, just as it does for locals then wrapped in spans.
There was a problem hiding this comment.
I don't see a reason why a stack probe is any more necessary in M1 than in M2, or in M3
We've discussed that in #52979
All stack buffers should get stack cookie by default, irrespective of the exact pattern used to create them. We may be missing it for some patterns today. ... we really should be performing stack checks more frequently. If the JIT is able to prove that all code operating on the buffer is safe with proper bounds checks, it should be ok to omit the stack cookie. I am not sure how often would this optimization help in practice.
I am not sure we can build any model in JIT like that, cc @dotnet/jit-contrib
There was a problem hiding this comment.
If we're serious about that, then we should fix the JIT to emit the cookie in all cases, including like M3. If M1 needs a cookie because, as quoted in that issue, someone may the use unsafe code to manipulate the span, then M2 and M3 do as well, and really any code where a ref is taken to a local and passed around. Otherwise, also quoting from that issue:
I agree that it is more reasonable to omit stack cookies for 100% safe Span-based code
There was a problem hiding this comment.
I've reverted the change since there is nothing we can do
There was a problem hiding this comment.
Current thinking is to re-examine the stack protections in the JIT for .NET 11.
Aside from the concerns above, we are placing increasing amounts of state on the stack that traditionally only lived in the heap.
src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Stephen Toub <stoub@microsoft.com>
|
@stephentoub anything else here? |
| @@ -178,6 +178,10 @@ private void ComputeHash(IncrementalHash sha256, string path, DateTime lastChang | |||
| sha256.AppendData(_byteBuffer, 0, length); | |||
| sha256.AppendData(Separator, 0, Separator.Length); | |||
|
|
|||
| #if NET | |||
There was a problem hiding this comment.
I think you could change this for all builds to be:
BinaryPrimitives.WriteInt64LittleEndian(_byteBuffer, lastChangedUtc.Ticks);The endianness shouldn't matter here, as it's just being used in an in-memory hash to determine whether something changes or not.
Could probably also use:
long ticks = lastChangedUtc.Ticks;
MemoryMarshal.Write(_byteBuffer, ref ticks);There was a problem hiding this comment.
Could probably also use:
I was avoiding this one because it is a candidate for "requires unsafe" due to its generic nature
| { | ||
| Write(new ReadOnlySpan<byte>(&value, 1)); | ||
| Write([value]); |
There was a problem hiding this comment.
No, Write accepts a ReadOnlySpan. It emits the equivalent of var tmp = value; Write(new ReadOnlySpan<byte>(in tmp));.
No description provided.