Skip to content

Test Utf8JsonReader code shape for ARM64 regression - #132430

Draft
jozkee wants to merge 18 commits into
mainfrom
jozkee-perf-bisect-131600-reader-patterns
Draft

Test Utf8JsonReader code shape for ARM64 regression#132430
jozkee wants to merge 18 commits into
mainfrom
jozkee-perf-bisect-131600-reader-patterns

Conversation

@jozkee

@jozkee jozkee commented Aug 17, 2026

Copy link
Copy Markdown
Member

Tests whether the compound pattern rewrites in the single-segment Utf8JsonReader affect the Linux ARM64 Perf_Get.GetUInt64 regression tracked by #131600.

This draft is stacked on #132399 only to retain the preserved #130976 source commits and the benchmark-image libunwind prerequisite. It does not modify #132399.

The benchmark should compare:

  • 66b30d95: last measured good source stage
  • 43560bc7: first measured bad source stage
  • e92960b3: Utf8JsonReader.cs compound patterns restored to their previous code shape

Validation:

  • dotnet build src/libraries/System.Text.Json/src/System.Text.Json.csproj
  • dotnet build /t:test src/libraries/System.Text.Json/tests/System.Text.Json.Tests/System.Text.Json.Tests.csproj
    • net11.0: 53,694 passed
    • net481: 53,382 passed

Note

This pull request was prepared with GitHub Copilot.

eiriktsarpalis and others added 8 commits July 20, 2026 16:24
Use C# 14 field-backed properties for private state used only by accessors, and expression-bodied members for single-expression methods.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 69ae6b80-f573-41bd-8cf9-e0f858510bbe
Replace built-in null comparisons and stable compound comparisons with equivalent C# patterns. Retain reflection comparisons that bind user-defined equality operators.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 69ae6b80-f573-41bd-8cf9-e0f858510bbe
Use the C# 14 field keyword while preserving mutable-schema validation.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 69ae6b80-f573-41bd-8cf9-e0f858510bbe
…dText.cs

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Revert the compound pattern rewrites in the single-segment reader to isolate the ARM64 GetUInt64 regression in #131600.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings August 17, 2026 21:06
@jozkee

This comment was marked as outdated.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 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.

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.

Pull request overview

This PR adjusts a few hot-path code shapes (notably in Utf8JsonReader) to help determine whether recent compound-pattern rewrites are implicated in the Linux/ARM64 System.Text.Json.Tests.Perf_Get.GetUInt64 regression tracked by #131600.

Changes:

  • Replaces several C# pattern-based comparisons in Utf8JsonReader with equivalent ==/!= and &&/|| forms to restore an earlier code shape.
  • Restores a pre-modernization ternary shape for JsonEncodedText.GetHashCode().
  • Updates NativeAOT’s libunwind symbol-privatization step to avoid invoking llvm-link as the relocatable linker by preferring ld.lld/ld when necessary.

Reviewed changes

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

File Description
src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs Rewrites several compound/pattern comparisons in number parsing and token classification to different boolean forms for code-shape testing.
src/libraries/System.Text.Json/src/System/Text/Json/JsonEncodedText.cs Changes GetHashCode() to a ternary form to match a prior code shape.
src/coreclr/nativeaot/Runtime/Full/CMakeLists.txt Adds linker selection logic for the libunwind privatization custom command to avoid using llvm-link.
Suppressed comments (4)

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

  • Use byte literals for these UTF-8 byte checks to avoid char-to-int promotions and to match the rest of the reader's byte-oriented parsing logic.
                if (nextByte != '.' && nextByte != 'E' && nextByte != 'e')
                {
                    _bytePositionInLine += i;
                    ThrowHelper.ThrowJsonReaderException(ref this, ExceptionResource.ExpectedEndOfDigitNotFound, nextByte);
                }
            }

            Debug.Assert(nextByte == '.' || nextByte == 'E' || nextByte == 'e');

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

  • These are byte values from the UTF-8 payload; comparing to char literals promotes to int. Prefer (byte)'E'/(byte)'e' here (and in the assert) to keep the parsing logic purely byte-based.
                if (nextByte != 'E' && nextByte != 'e')
                {
                    _bytePositionInLine += i;
                    ThrowHelper.ThrowJsonReaderException(ref this, ExceptionResource.ExpectedNextDigitEValueNotFound, nextByte);
                }
            }

            Debug.Assert(nextByte == 'E' || nextByte == 'e');

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

  • Use byte literals for these UTF-8 byte comparisons to avoid implicit promotions and keep parsing code consistent with other (byte)'0' usage in this file.
            nextByte = data[i];
            if (nextByte != '.' && nextByte != 'E' && nextByte != 'e')
            {

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

  • Since nextByte is a byte from the UTF-8 data, prefer comparing against byte literals to avoid implicit numeric promotion (and to keep the parsing logic byte-oriented).
            byte nextByte = data[i];
            if (nextByte == '+' || nextByte == '-')
            {

@jozkee

This comment was marked as outdated.

jozkee and others added 10 commits August 17, 2026 16:47
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jozkee

jozkee commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

Comparing the pattern-rewrite commit with the current PR head, which selectively restores the Utf8JsonReader expressions. Both full SHAs were verified against PR metadata and local Git.

@EgorBot -ubuntu24_azure_ampere -pr 132430 -commits 43560bc --filter "System.Text.Json.Tests.Perf_Get.GetUInt64"

Note

This benchmark request was prepared with GitHub Copilot.

Copilot AI review requested due to automatic review settings August 17, 2026 23:41
@jozkee

jozkee commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

First-pass cumulative bisection of the confirmed Utf8JsonReader code-shape regression. The stages split the ten one-line restores into candidate groups of 1, 4, 3, and 2 changes. Every full SHA was verified against the current PR history and local Git.

@EgorBot -ubuntu24_azure_ampere -pr 132430 -commits d354506,c514b6e24c36c921ccaf7ea6c95f528c52694b63,9d37db381a99a26be0e757f6aa56f96b9adf429c --filter "System.Text.Json.Tests.Perf_Get.GetUInt64"

Note

This benchmark request was prepared with GitHub Copilot.

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.

Pull request overview

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

Suppressed comments (1)

src/coreclr/nativeaot/Runtime/Full/CMakeLists.txt:44

  • ld.lld is advertised as the preferred linker when CMAKE_LINKER resolves to llvm-link, but the current find_program(... NO_DEFAULT_PATH) only searches the llvm-link directory. If ld.lld is available on PATH (or via CMake defaults) but not colocated with llvm-link, the build will silently fall back to ld (or even fail if ld isn’t present) despite ld.lld being available. Consider falling back to a default-path find_program for ld.lld before trying ld.
    find_program(NATIVEAOT_PRIVATE_LIBUNWIND_LD_LLD
      NAMES "ld.lld${NATIVEAOT_PRIVATE_LIBUNWIND_LINKER_VERSION}${NATIVEAOT_PRIVATE_LIBUNWIND_LINKER_EXTENSION}" ld.lld
      PATHS "${NATIVEAOT_PRIVATE_LIBUNWIND_TOOL_DIR}"
      NO_DEFAULT_PATH)

@jozkee

jozkee commented Aug 18, 2026

Copy link
Copy Markdown
Member Author

Final split for the recovery boundary from issue #509. fa936cfa restores IsTokenTypeString, which is used by ValueTextEquals; 69b2a7cb restores the Release-effective number-terminator condition in TryGetNumber. Every full SHA was verified against the current PR history and local Git.

@EgorBot -ubuntu24_azure_ampere -pr 132430 -commits d354506,fa936cfa0ec1e7d1f634fa8885fd3a5519b2f432,69b2a7cbfc7dfc8031deacace4bd770255522706 --filter "System.Text.Json.Tests.Perf_Get.GetUInt64"

Note

This benchmark request was prepared with GitHub Copilot.

@jozkee

jozkee commented Aug 18, 2026

Copy link
Copy Markdown
Member Author

ARM64 optimized disassembly at the confirmed regression boundary. This limits BenchmarkDotNet to one warmup and one measured invocation while requesting disassembly for TryGetNumber and GetUInt64. Both full SHAs were verified against the published PR history and local Git.

@EgorBot -ubuntu24_azure_ampere -pr 132430 -commits fa936cf,69b2a7cbfc7dfc8031deacace4bd770255522706 --filter "System.Text.Json.Tests.Perf_Get.GetUInt64" --warmupCount 1 --iterationCount 1 --minIterationCount 1 --maxIterationCount 1 --invocationCount 1 --unrollFactor 1 --envvars "DOTNET_JitDisasm:TryGetNumber GetUInt64" DOTNET_JitDisasmDiffable:1 DOTNET_ReadyToRun:0 DOTNET_TieredCompilation:0

Note

This benchmark request was prepared with GitHub Copilot.

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.

3 participants