Cleanup exception throwing in Utf8JsonWriter

[pentp]

• Remove some unused throw helpers and exception strings.
• Make all throw helpers visible as not-returning to JIT so that the callsites can be properly optimized.
• Refactor some duplicated throw helpers.

This addresses #111332 (comment)

[Copilot]

Pull Request Overview

This PR refactors exception throwing helpers in Utf8JsonWriter and related components to remove unused helpers, improve JIT optimization by marking throw methods as not-returning, and consolidate duplicated code.

• Refactored throw helper methods to use new patterns (e.g. GetValidateStartFailedException)
• Replaced ValidatePropertyAndDepth calls with separate property and depth validations
• Updated Utf8JsonReader and MultiSegment handling to follow the new exception patterns

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.cs	Refactored property getters and exception throwing methods
src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteValues.Raw.cs	Updated calls to throw helpers for raw value validation
src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteValues.Helpers.cs	Consolidated validation helpers for writing values
src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteProperties.String.cs	Updated property and value validations in string writing methods
src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteProperties.Helpers.cs	Replaced ValidatePropertyNameAndDepth with separate validations
src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteProperties.Bytes.cs	Replaced property name length validation calls with updated methods
src/libraries/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.cs	Removed unused composite validation methods
src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.cs	Removed redundant throw helpers and updated exception construction patterns
src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs	Updated literal validation to use new exception patterns
src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs	Adjusted multi-segment literal validation to use scoped Span parameters
src/libraries/System.Text.Json/src/Resources/Strings.resx	Removed obsolete string resources

[eiriktsarpalis]

Thanks, but I think I need to better understand what the motivation for filing this PR is. As it stands I think this should be split into at least two separate PRs (one eliminating dead code, and one proposing changes to the helpers) so that each can be reviewed in it own merits.

[pentp]

There's not much dead code actually, only the 3 strings (and 3 throw helpers). Everything else is about removing NoInlining from throw helpers and making them small enough that the JIT actually sees that they are throwing (never returning) and optimizes all callsites as cold code.
This gives significant code size wins, I can try running the STJ benchmarks also.

[jkotas]

I can try running the STJ benchmarks also.

Yes, we like to see numbers for any perf related changes.

[pentp]

I was waiting on #115918, but I guess I could get some perf numbers regardless of that issue.

[pentp]

I addressed the feedback, but this PR now depends on #118280 getting merged first. Will gather some perf numbers also now.

[pentp]

@MihuBot benchmark System.Text.Json

[jeffhandley]

@pentp There's one outstanding thread to resolve. And if you could summarize the benchmark results here that would help us get this moved forward too. We appreciate the pre-requisite work you've done to move this through the blockers it had.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

[jeffhandley]

@MihuBot benchmark System.Text.Json

[jeffhandley]

The previous perf run by MihuBot showed a mixture of improvements and regressions. Let's rerun to get some fresh numbers.

[MihaZupan]

https://gist.github.com/MihuBot/af3bf583aeca8f64c63d0d575a35fcbf

[jeffhandley]

@eiriktsarpalis -- What do you think about the perf results? Do you think the changes in this PR could actually be leading to these regressions?

Performance Regressions > 3% (PR vs Main)

Benchmark Suite	Method	Parameters	PR Mean	Ratio	Δ
Perf_Depth	ReadSpanEmptyLoop	Depth=65	1,762 ns	1.69	+69% ⚠️
WriteJson ImmutableDictionary	SerializeToWriter	SourceGen	7.08 μs	1.20	+20%
ReadJson HashSet<String>	DeserializeFromString	SourceGen	5.51 μs	1.13	+13%
Perf_Reader	ReadSingleSpanSeqEmptyLoop	Compact=F, HelloWorld	66.3 ns	1.12	+12%
ReadJson ArrayList	DeserializeFromReader	SourceGen	12.15 μs	1.12	+12%
ReadJson Nullable<DateTimeOffset>	DeserializeFromUtf8Bytes	SourceGen	70.7 ns	1.11	+11%
ReadJson ArrayList	DeserializeFromStream	Reflection	10.42 μs	1.09	+9%
Perf_Strings	WriteStringsUtf8	Fmt=T, Skip=F, NoneEsc	2.23 ms	1.08	+8%
ReadJson TreeRecord	DeserializeFromString	Reflection	13.70 μs	1.08	+8%
ReadJson Nullable<DateTimeOffset>	DeserializeFromString	Reflection	81.6 ns	1.08	+8%
ReadJson Hashtable	DeserializeFromReader	Reflection	19.20 μs	1.08	+8%
Perf_Reader	ReadSingleSpanSeqEmptyLoop	Compact=F, BroadTree	5,753 ns	1.07	+7%
Perf_Base64	HeavyEscaping	100 bytes	35.0 ns	1.07	+7%
Perf_Base64	NoEscaping	1000 bytes	68.2 ns	1.07	+7%
WriteJson ImmutableSortedDict	SerializeToStream	Reflection	4.47 μs	1.07	+7%
WriteJson Location	SerializeToStream	Reflection	433 ns	1.07	+7%
ReadJson StructRecord	DeserializeFromString	Reflection	342 ns	1.07	+7%
ReadJson LargeStruct	DeserializeFromUtf8Bytes	SourceGen	490 ns	1.07	+7%
ReadJson HashSet<String>	DeserializeFromStream	SourceGen	5.77 μs	1.07	+7%
Perf_Strings	WriteStringsUtf8	Fmt=T, Skip=T, NoneEsc	2.20 ms	1.06	+6%
WriteJson BinaryData	SerializeToStream	SourceGen	158 ns	1.06	+6%
ReadJson ClassRecord	DeserializeFromStream	Reflection	578 ns	1.06	+6%
Doc EnumerateObject	PropertyIndexer	NumericProperties	1,338 ns	1.06	+6%
ReadJson BinaryData	DeserializeFromString	Reflection	341 ns	1.05	+5%
ReadJson LoginViewModel	DeserializeFromStream	SourceGen	368 ns	1.05	+5%
Node Perf_Create	Create_JsonArray	—	630 ns	1.05	+5%
Doc EnumerateObject	PropertyIndexer	StringProperties	1,339 ns	1.05	+5%
Perf_Segment	ReadMultiSegmentSequence	Json40KB, seg=4096	36.5 μs	1.04	+4%
Perf_Segment	ReadSingleSegmentSequence	Json40KB	32.1 μs	1.04	+4%
Perf_Segment	ReadSingleSegmentSequence	Json400KB	322 μs	1.04	+4%
Perf_Reader	ReadSingleSpanSeqEmptyLoop	Compact=F, Json400B	403 ns	1.04	+4%
WriteJson Int32	SerializeToStream	SourceGen	110 ns	1.04	+4%
WriteJson ClassRecord	SerializeToStream	SourceGen	178 ns	1.04	+4%
WriteJson Nullable<DateTimeOffset>	SerializeToString	SourceGen	106 ns	1.04	+4%
WriteJson Dictionary<String,String>	SerializeObjectProperty	SourceGen	3.45 μs	1.04	+4%
ReadJson ClassRecord	DeserializeFromString	SourceGen	457 ns	1.04	+4%
Reader Perf_Base64	HeavyEscaping	100 bytes	50.6 ns	1.04	+4%

Note: The +69% Perf_Depth regression at Depth=65 has a very large error margin (±645 ns on 1,762 ns mean), suggesting it may be measurement noise. The +20% ImmutableDictionary.SerializeToWriter(SourceGen) and +13% HashSet<String>.DeserializeFromString(SourceGen) are the most significant reliable regressions.

[eiriktsarpalis]

What do you think about the perf results? Do you think the changes in this PR could actually be leading to these regressions?

Possibly. The top ones seem diverge substantially enough to not be random fluctuations. Does the distribution of improvements look similar on the other side?

[pentp]

It's possible that some JIT or library changes over the last 10 months have caused this to bit-rot somewhat, I should probably re-validate if the individual changes still make sense.
But the regressions look odd, considering almost all changes here either reduce code size or enable JIT to recognize some of it as cold code. Maybe it has unintended consequences on inlining decisions on some critical path.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

---

You can also share your feedback on Copilot code review. Take the survey.

[pentp]

@MihuBot benchmark System.Text.Json

[MihuBot]

See benchmark results at https://gist.github.com/MihuBot/eefca0784f771d50134e128ab51441df