-
Notifications
You must be signed in to change notification settings - Fork 5k
Reduce unsafe from STJ #114154
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
Reduce unsafe from STJ #114154
Conversation
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
...libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.DbRow.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonElement.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.Escaping.cs
Show resolved
Hide resolved
...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.DbRow.cs
Show resolved
Hide resolved
TypeCode.Int16 => (ulong)(short)(object)value, | ||
TypeCode.UInt16 => (ushort)(object)value, | ||
TypeCode.SByte => (ulong)(sbyte)(object)value, | ||
_ => (byte)(object)value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous code was asserting in the drain, and you're not. If a new TypeCode were to be introduced, is the exception that comes out of this cast going to be useful enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous code was asserting in the drain, and you're not. If a new TypeCode were to be introduced, is the exception that comes out of this cast going to be useful enough?
if a new type code is added, the default will throw an InvalidCastException? we can only unbox byte out of a boxed byte-enum.
PS: unfortunately, C# does not support statemens in pattern-match switch
@eiriktsarpalis @stephentoub does it look good as is? I've reverted the SourceWriter change |
src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonElement.cs
Outdated
Show resolved
Hide resolved
internal static unsafe string CharsToStringClass(ReadOnlySpan<char> chars) | ||
internal static | ||
#if !NET | ||
unsafe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is safe for callers. Move this unsafe
into the method so that it wraps the unsafe code only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up duplicating code since netfx needs unsafe context in two places
Addressed the feedback, thanks! |
Let's see if we can make System.Text.Json 100% unsafe code free (including what we might mark as "requires unsafe" via dotnet/designs#330), could be a good example for community lib authors who heavily rely on Unsafe when implementing serializers.
There are few things left, but they're doable, except for these two:
[module: SkipLocalsInit]
requires AllowUnsafeBlocks, but I suspect we never measured perf impact of it, for STJ specifically. It mainly just improves non-constant stackallocs. STJ doesn't have those. It uses constant-sized (256 bytes) ones. 256 bytes should be zeroeable with a few instructions (unrolled) - still not free, but perhaps if it doesn't have an impact on all of our dotnet/performance benchmarks we can remove it?TextEncoder.FindFirstCharacterToEncodeUtf8(Span<byte>)
but no safe one for UTF-16. I'll file an API proposalI did not touch
unsafe
for .NETStandard/older TFMs, was focusing on .NET 10.0 STJ.