Skip to content

Revert "[System.Text.Json] Fix ARM64 Utf8JsonReader regression" - #133099

Merged
jozkee merged 2 commits into
mainfrom
revert-132452-fix-utf8jsonreader-arm64-pattern-regression
Sep 10, 2026
Merged

Revert "[System.Text.Json] Fix ARM64 Utf8JsonReader regression"#133099
jozkee merged 2 commits into
mainfrom
revert-132452-fix-utf8jsonreader-arm64-pattern-regression

Conversation

@jozkee

@jozkee jozkee commented Sep 2, 2026

Copy link
Copy Markdown
Member

Reverts #132452 now that we've ingested roslyn fix.

Copilot AI lite review requested due to automatic review settings September 2, 2026 16:02
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@jozkee

jozkee commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

@EgorBot -ubuntu24_azure_ampere --filter "System.Text.Json.Tests.Perf_Get.GetUInt64"

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

It reintroduces pattern-matching forms that were previously reverted specifically to fix a large Linux ARM64 performance regression, so it needs either restored explicit comparisons or new evidence that the regression no longer applies.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR reverts the explicit-comparison changes from #132452 by switching several hot-path byte comparisons in Utf8JsonReader (single- and multi-segment) and number validation back to C# pattern-matching forms.

Changes:

  • Replaces != / == chains with is / is not pattern matching for . / e / E terminators and + / - sign checks in Utf8JsonReader (single- and multi-segment).
  • Updates comment marker validation in multi-segment reader to use is not (A or B).
  • Updates JsonWriterHelper.ValidateNumber exponent/sign checks to use pattern matching.
File summaries
File Description
src/libraries/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.cs Switches exponent/sign byte checks in ValidateNumber back to pattern matching.
src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs Switches multiple terminator/sign/comment-marker checks back to pattern matching.
src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs Switches multiple terminator/sign checks back to pattern matching in the single-span reader.
Review details

Suppressed comments (7)

src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs:1532

  • This reintroduces the pattern-matching exponent check that was reverted for a large Linux ARM64 regression in #132452. Unless there is new perf evidence showing the JIT/codegen issue is resolved, keep the explicit comparisons here.
                if (nextByte is not ((byte)'E' or (byte)'e'))

src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs:1627

  • This restores the pattern-matching form for the leading-zero terminator check which was previously changed back to explicit comparisons to address a Linux ARM64 perf regression (#132452). Consider keeping the explicit comparisons unless there is new perf data showing the regression is gone.
            if (nextByte is not ((byte)'.' or (byte)'E' or (byte)'e'))

src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs:1706

  • This reintroduces the pattern-matching sign check that was changed back to explicit comparisons to address a Linux ARM64 perf regression (#132452). Consider keeping the explicit comparisons unless there is new perf evidence showing the regression is resolved.
            if (nextByte is (byte)'+' or (byte)'-')

src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs:1203

  • This reintroduces the pattern-matching exponent check that was reverted for a large Linux ARM64 regression in #132452. Unless there is new perf evidence showing the underlying codegen issue is resolved, keep the explicit comparisons here.
                if (nextByte is not ((byte)'E' or (byte)'e'))

src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs:1344

  • This restores the pattern-matching form for the leading-zero terminator check which was previously changed back to explicit comparisons to address a Linux ARM64 perf regression (#132452). Consider keeping the explicit comparisons unless there is new perf data showing the regression is gone.
            if (nextByte is not ((byte)'.' or (byte)'E' or (byte)'e'))

src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs:1493

  • This reintroduces the pattern-matching sign check that was changed back to explicit comparisons to address a Linux ARM64 perf regression (#132452). Consider keeping the explicit comparisons unless there is new perf evidence showing the regression is resolved.
            if (nextByte is (byte)'+' or (byte)'-')

src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs:2267

  • This reintroduces the pattern-matching comment marker check that was changed back to explicit comparisons to address a Linux ARM64 perf regression (#132452). Consider keeping the explicit comparisons unless there is new perf evidence showing the regression is resolved.
            if (marker is not (JsonConstants.Slash or JsonConstants.Asterisk))
  • Files reviewed: 3/3 changed files
  • Comments generated: 4
  • Review effort level: Lite

@jozkee

jozkee commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

@jjonescz, looks like we still have the regression EgorBot/Benchmarks#564 (comment).

@jjonescz

jjonescz commented Sep 3, 2026

Copy link
Copy Markdown
Member

Are you sure it is using the correct compiler? With the new compiler, the IL of these is patterns is equivalent to the explicit == comparisons. It looks like the benchmark harness reuses the same artifacts folder for both the baseline and the PR runs without any cleanup in between, so there could be something wrong there? To investigate further, would it be possible to get the DLLs for inspection?

@jozkee

jozkee commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

@EgorBot -ubuntu24_azure_ampere --filter "System.Text.Json.Tests.Perf_Get.GetUInt64" --envvars DOTNET_TieredPGO:0

@jozkee

jozkee commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

@EgorBot -ubuntu24_azure_ampere --filter "System.Text.Json.Tests.Perf_Get.GetUInt64" --envvars DOTNET_TieredCompilation:0

@jozkee

jozkee commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

would it be possible to get the DLLs for inspection?

I don't think that's possible, @EgorBo? I'm thinking it could be JIT-related so I'm doing a few more runs.

Copilot AI review requested due to automatic review settings September 3, 2026 16:09
@jozkee

jozkee commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

Making another default run to make sure the regression is consistent.

@EgorBot -ubuntu24_azure_ampere --filter "System.Text.Json.Tests.Perf_Get.GetUInt64"

EDIT: it failed with what it appears like a transient bug EgorBot/Benchmarks#567 (comment).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The changes are localized, preserve the existing parsing logic, and primarily revert comparison forms back to equivalent pattern-matching expressions.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@jozkee

jozkee commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

@EgorBot -ubuntu24_azure_ampere --filter "System.Text.Json.Tests.Perf_Get.GetUInt64" --envvars DOTNET_ReadyToRun:0

@jozkee

jozkee commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

@EgorBot -ubuntu24_azure_ampere --filter "System.Text.Json.Tests.Perf_Get.GetUInt64"

@jozkee

jozkee commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

This PR does not show a deterministic source or codegen regression. The observed difference is attributable to stale static-PGO/ReadyToRun data and reverses between runs. We can merge the PR, then validate the benchmark after the refreshed dotnet-optimization dependency flows into main.

PR #132387 or a future update may address stale profiles for release/11.0.

@jozkee

jozkee commented Sep 10, 2026

Copy link
Copy Markdown
Member Author

/ba-g #131990

@jozkee
jozkee merged commit 2510f81 into main Sep 10, 2026
79 of 82 checks passed
@jozkee
jozkee deleted the revert-132452-fix-utf8jsonreader-arm64-pattern-regression branch September 10, 2026 06:46
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.

4 participants