Skip to content

Conversation

@pentp
Copy link
Contributor

@pentp pentp commented May 18, 2025

  • 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 AI review requested due to automatic review settings May 18, 2025 23:08
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 18, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

pentp commented May 19, 2025

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
Copy link
Member

jkotas commented May 19, 2025

I can try running the STJ benchmarks also.

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

@jkotas jkotas added the tenet-performance Performance related issue label May 19, 2025
@eiriktsarpalis eiriktsarpalis added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 1, 2025
@pentp
Copy link
Contributor Author

pentp commented Jul 16, 2025

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

@dotnet-policy-service dotnet-policy-service bot removed needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity labels Jul 16, 2025
@eiriktsarpalis eiriktsarpalis added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 17, 2025
@dotnet-policy-service dotnet-policy-service bot removed needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity labels Aug 3, 2025
@pentp
Copy link
Contributor Author

pentp commented Aug 3, 2025

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

@pentp
Copy link
Contributor Author

pentp commented Aug 3, 2025

@MihuBot benchmark System.Text.Json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Text.Json community-contribution Indicates that the PR has been added by a community member tenet-performance Performance related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants