Uh oh!
There was an error while loading. Please reload this page.
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 == '-')
{
Uh oh!
There was an error while loading. Please reload this page.
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>
jozkee
commented
Aug 17, 2026
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. |
jozkee
commented
Aug 17, 2026
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)
jozkee
commented
Aug 18, 2026
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. |
jozkee
commented
Aug 18, 2026
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. |
jozkee
commented
Aug 18, 2026
Retrying the ARM64 disassembly run after issue #511 was rejected because @EgorBot -ubuntu24_azure_ampere -pr 132430 -commits fa936cf,69b2a7cbfc7dfc8031deacace4bd770255522706 --filter "System.Text.Json.Tests.Perf_Get.GetUInt64" --warmupCount 1 --iterationCount 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. |
jozkee
commented
Aug 19, 2026
Running the three System.Text.Json benchmarks added by dotnet/performance commit @EgorBot -ubuntu24_azure_ampere -pr 132430 -commits fa936cf,69b2a7cbfc7dfc8031deacace4bd770255522706 --filter "Perf_ElementParseValue" "Perf_CommentLineSeparators" "Perf_ValueTextEquals" Note This benchmark request was prepared with GitHub Copilot. |
jozkee
commented
Aug 19, 2026
Running the three System.Text.Json benchmarks from dotnet/performance commit @EgorBot -ubuntu24_azure_ampere -pr 132504 --filter "Perf_ElementParseValue" "Perf_CommentLineSeparators" "Perf_ValueTextEquals" usingSystem;usingSystem.Buffers;usingSystem.Text;usingSystem.Text.Json;usingBenchmarkDotNet.Attributes;publicclassPerf_ElementParseValue{privatebyte[]_string=null!;privatebyte[]_number=null!;privatebyte[]_object=null!;[GlobalSetup]publicvoidSetup(){_string=Encoding.UTF8.GetBytes("\"a short json string value\"");_number=Encoding.UTF8.GetBytes("123456789");_object=Encoding.UTF8.GetBytes("{\"value\":123456789}");}[Benchmark]publicJsonValueKindParseString()=>Parse(_string);[Benchmark]publicJsonValueKindParseNumber()=>Parse(_number);[Benchmark]publicJsonValueKindParseObject()=>Parse(_object);privatestaticJsonValueKindParse(byte[]utf8Json){varreader=newUtf8JsonReader(utf8Json);returnJsonElement.ParseValue(refreader).ValueKind;}}publicclassPerf_CommentLineSeparators{privateconstintSegmentSize=100;[Params(JsonCommentHandling.Skip,JsonCommentHandling.Allow)]publicJsonCommentHandlingCommentHandling;[Params(false,true)]publicboolMultiSegment;privatebyte[]_jsonPayload=null!;privateReadOnlySequence<byte>_jsonPayloadSequence;[GlobalSetup]publicvoidSetup(){_jsonPayload=Encoding.UTF8.GetBytes("{}//"+newstring('\u2027',2000)+"\n");_jsonPayloadSequence=SequenceFactory.Create(_jsonPayload,SegmentSize);}[Benchmark]publicvoidReadCommentWithSeparators(){varstate=newJsonReaderState(newJsonReaderOptions{CommentHandling=CommentHandling});Utf8JsonReaderreader=MultiSegment?newUtf8JsonReader(_jsonPayloadSequence,isFinalBlock:true,state):newUtf8JsonReader(_jsonPayload,isFinalBlock:true,state);while(reader.Read()){}}}publicclassPerf_ValueTextEquals{privateconstintPropertyCount=100;[Params(false,true)]publicboolEscaped;[Params(false,true)]publicboolMultiSegment;privatebyte[]_dataUtf8=null!;privateReadOnlySequence<byte>_sequence;privatebyte[]_lookupUtf8=null!;[GlobalSetup]publicvoidSetup(){_lookupUtf8=Encoding.UTF8.GetBytes("property_"+PropertyCount);varbuilder=newStringBuilder("{");for(inti=0;i<PropertyCount;i++){if(i!=0){builder.Append(',');}builder.Append('"');AppendPropertyName(builder,i,Escaped);builder.Append("\":");builder.Append(i);}builder.Append('}');_dataUtf8=Encoding.UTF8.GetBytes(builder.ToString());_sequence=SequenceFactory.Create(_dataUtf8,_dataUtf8.Length/2);}[Benchmark]publicintMatchPropertyNames(){Utf8JsonReaderreader=MultiSegment?newUtf8JsonReader(_sequence):newUtf8JsonReader(_dataUtf8);intmatches=0;while(reader.Read()){if(reader.TokenType==JsonTokenType.PropertyName&&reader.ValueTextEquals(_lookupUtf8)){matches++;}}returnmatches;}privatestaticvoidAppendPropertyName(StringBuilderbuilder,intindex,boolescaped){conststringPrefix="property_";if(!escaped){builder.Append(Prefix).Append(index);return;}foreach(charcinPrefix){builder.Append("\\u").Append(((int)c).ToString("x4"));}builder.Append(index);}}internalstaticclassSequenceFactory{publicstaticReadOnlySequence<byte>Create(byte[]data,intsegmentSize){varfirst=newBufferSegment(data.AsMemory(0,Math.Min(segmentSize,data.Length)));BufferSegmentlast=first;for(intoffset=segmentSize;offset<data.Length;offset+=segmentSize){last=last.Append(data.AsMemory(offset,Math.Min(segmentSize,data.Length-offset)));}returnnewReadOnlySequence<byte>(first,0,last,last.Memory.Length);}privatesealedclassBufferSegment:ReadOnlySequenceSegment<byte>{publicBufferSegment(ReadOnlyMemory<byte>memory){Memory=memory;}publicBufferSegmentAppend(ReadOnlyMemory<byte>memory){varsegment=newBufferSegment(memory){RunningIndex=RunningIndex+Memory.Length};Next=segment;returnsegment;}}}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.