Skip to content

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

Merged
merged 8 commits into from
Apr 10, 2025
Merged

Reduce unsafe from STJ #114154

merged 8 commits into from
Apr 10, 2025

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Apr 2, 2025

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:

  1. [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?
  2. Lack of TextEncoder.FindFirstCharacterToEncode(Span) overload. What is interesting, that there is TextEncoder.FindFirstCharacterToEncodeUtf8(Span<byte>) but no safe one for UTF-16. I'll file an API proposal

I did not touch unsafe for .NETStandard/older TFMs, was focusing on .NET 10.0 STJ.

Copy link
Contributor

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

TypeCode.Int16 => (ulong)(short)(object)value,
TypeCode.UInt16 => (ushort)(object)value,
TypeCode.SByte => (ulong)(sbyte)(object)value,
_ => (byte)(object)value
Copy link
Member

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?

Copy link
Member Author

@EgorBo EgorBo Apr 2, 2025

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

@EgorBo
Copy link
Member Author

EgorBo commented Apr 9, 2025

@eiriktsarpalis @stephentoub does it look good as is? I've reverted the SourceWriter change

internal static unsafe string CharsToStringClass(ReadOnlySpan<char> chars)
internal static
#if !NET
unsafe
Copy link
Member

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?

Copy link
Member Author

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

@EgorBo
Copy link
Member Author

EgorBo commented Apr 9, 2025

Addressed the feedback, thanks!

@stephentoub stephentoub changed the title Remove unsafe from STJ Reduce unsafe from STJ Apr 9, 2025
@EgorBo EgorBo merged commit aa458fa into dotnet:main Apr 10, 2025
86 of 88 checks passed
@EgorBo EgorBo deleted the safe-stj-1 branch April 10, 2025 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants