Test Utf8JsonReader code shape for ARM64 regression - #132430
Conversation
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>
This comment was marked as outdated.
This comment was marked as outdated.
|
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. |
|
Tagging subscribers to this area: @dotnet/area-system-text-json |
There was a problem hiding this comment.
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
Utf8JsonReaderwith 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-linkas the relocatable linker by preferringld.lld/ldwhen 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 == '-')
{
This comment was marked as outdated.
This comment was marked as outdated.
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>
|
Comparing the pattern-rewrite commit with the current PR head, which selectively restores the @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. |
|
First-pass cumulative bisection of the confirmed @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. |
There was a problem hiding this comment.
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.lldis advertised as the preferred linker whenCMAKE_LINKERresolves tollvm-link, but the currentfind_program(... NO_DEFAULT_PATH)only searches thellvm-linkdirectory. Ifld.lldis available onPATH(or via CMake defaults) but not colocated withllvm-link, the build will silently fall back told(or even fail ifldisn’t present) despiteld.lldbeing available. Consider falling back to a default-pathfind_programforld.lldbefore tryingld.
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)
|
Final split for the recovery boundary from issue #509. @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. |
|
ARM64 optimized disassembly at the confirmed regression boundary. This limits BenchmarkDotNet to one warmup and one measured invocation while requesting disassembly for @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. |
Tests whether the compound pattern rewrites in the single-segment
Utf8JsonReaderaffect the Linux ARM64Perf_Get.GetUInt64regression 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 stage43560bc7: first measured bad source stagee92960b3:Utf8JsonReader.cscompound patterns restored to their previous code shapeValidation:
dotnet build src/libraries/System.Text.Json/src/System.Text.Json.csprojdotnet build /t:test src/libraries/System.Text.Json/tests/System.Text.Json.Tests/System.Text.Json.Tests.csprojNote
This pull request was prepared with GitHub Copilot.