Intrinsicify JsonReaderHelper.IndexOfOrLessThan by benaadams · Pull Request #40877 · dotnet/runtime

benaadams · 2020-08-15T05:54:21Z

Update to use newer Sse2, Axv2 intrinsics; and add ARM as per the method it mimics (with changes) from SpanHelpers
Also add size optimization/simplification for wasm

x64 performance changes (up to +20% improvement)

better: 30, geomean: 1.113
worse: 1, geomean: 1.066
total diff: 31
| Slower | diff/base | Base Median (ns) | Diff Median (ns) | Modality|
| -------------------------------------------------------------------------------- | ---------:| ----------------:| ----------------:| --------:|
| System.Text.Json.Serialization.Tests.ReadJson<SimpleStructWithProperties>.Deseri | 1.07 | 288.23 | 307.28 | |
| Faster | base/diff | Base Median (ns) | Diff Median (ns) | Modality|
| -------------------------------------------------------------------------------- | ---------:| ----------------:| ----------------:| --------:|
| System.Text.Json.Serialization.Tests.ReadJson<Dictionary<String, String>>.Deseri | 1.22 | 22031.18 | 18102.24 | |
| System.Text.Json.Serialization.Tests.ReadJson<Dictionary<String, String>>.Deseri | 1.20 | 21554.46 | 17944.26 | |
| System.Text.Json.Serialization.Tests.ReadJson<MyEventsListerViewModel>.Deseriali | 1.20 | 341085.15 | 284277.87 | |
| System.Text.Json.Serialization.Tests.ReadJson<IndexViewModel>.DeserializeFromUtf | 1.19 | 29706.85 | 24903.98 | |
| System.Text.Json.Serialization.Tests.ReadJson<MyEventsListerViewModel>.Deseriali | 1.19 | 330481.64 | 277947.73 | |
| System.Text.Json.Serialization.Tests.ReadJson<MyEventsListerViewModel>.Deseriali | 1.19 | 399397.91 | 336085.63 | |
| System.Text.Json.Serialization.Tests.ReadJson<IndexViewModel>.DeserializeFromStr | 1.18 | 31986.63 | 27064.34 | |
| System.Text.Json.Serialization.Tests.ReadJson<Dictionary<String, String>>.Deseri | 1.18 | 23919.94 | 20268.43 | |
| System.Text.Json.Serialization.Tests.ReadJson<IndexViewModel>.DeserializeFromStr | 1.18 | 31000.14 | 26348.41 | |
| System.Text.Json.Serialization.Tests.ReadJson<LoginViewModel>.DeserializeFromStr | 1.15 | 522.04 | 454.92 | |
| System.Text.Json.Serialization.Tests.ReadJson<Hashtable>.DeserializeFromUtf8Byte | 1.12 | 68993.04 | 61636.12 | |
| System.Text.Json.Serialization.Tests.ReadJson<HashSet<String>>.DeserializeFromUt | 1.10 | 12663.49 | 11477.42 | |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableSortedDictionary<String, | 1.10 | 78887.97 | 71927.75 | |
| System.Text.Json.Serialization.Tests.ReadJson<Hashtable>.DeserializeFromStream | 1.10 | 69968.66 | 63797.29 | |
| System.Text.Json.Serialization.Tests.ReadJson<LoginViewModel>.DeserializeFromStr | 1.09 | 763.93 | 698.34 | |
| System.Text.Json.Serialization.Tests.ReadJson<HashSet<String>>.DeserializeFromSt | 1.09 | 12745.33 | 11734.38 | |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableDictionary<String, String | 1.09 | 48372.57 | 44572.25 | |
| System.Text.Json.Serialization.Tests.ReadJson<ArrayList>.DeserializeFromStream | 1.09 | 51673.60 | 47617.28 | |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableDictionary<String, String | 1.08 | 51226.78 | 47249.65 | |
| System.Text.Json.Serialization.Tests.ReadJson<HashSet<String>>.DeserializeFromSt | 1.08 | 13979.59 | 12973.07 | |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableDictionary<String, String | 1.08 | 48102.61 | 44652.30 | |
| System.Text.Json.Serialization.Tests.ReadJson<BinaryData>.DeserializeFromStream | 1.07 | 807.95 | 755.26 | |
| System.Text.Json.Serialization.Tests.ReadJson<ArrayList>.DeserializeFromString | 1.07 | 49947.46 | 46735.64 | |
| System.Text.Json.Serialization.Tests.ReadJson<BinaryData>.DeserializeFromUtf8Byt | 1.07 | 488.59 | 458.07 | |
| System.Text.Json.Serialization.Tests.ReadJson<LoginViewModel>.DeserializeFromUtf | 1.06 | 426.56 | 400.78 | |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableSortedDictionary<String, | 1.06 | 78155.94 | 73498.13 | |
| System.Text.Json.Serialization.Tests.ReadJson<Hashtable>.DeserializeFromString | 1.06 | 67906.38 | 63896.07 | |
| System.Text.Json.Serialization.Tests.ReadJson<ArrayList>.DeserializeFromUtf8Byte | 1.06 | 50083.93 | 47157.52 | |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableSortedDictionary<String, | 1.05 | 73213.63 | 69703.93 | |
| System.Text.Json.Serialization.Tests.ReadJson<Int32>.DeserializeFromUtf8Bytes | 1.03 | 90.42 | 87.57 | |

/cc @kunalspathak @jeffhandley for the intrinsics

gfoidl · 2020-08-15T09:12:30Z

For the following methods I see this pattern quite often. Maybe it's time to make these a public api?
Yeah, they're still easy to type helpers, but repeated often enough. Furthermore there could be asserts that the reads are within bounds, etc.
Most similar issue I've found is #36182

benaadams · 2020-08-17T01:50:19Z

better: 30, geomean: 1.113
worse: 1, geomean: 1.066
total diff: 31
| Slower | diff/base | Base Median (ns) | Diff Median (ns) | Modality|
| -------------------------------------------------------------------------------- | ---------:| ----------------:| ----------------:| --------:|
| System.Text.Json.Serialization.Tests.ReadJson<SimpleStructWithProperties>.Deseri | 1.07 | 288.23 | 307.28 | |
| Faster | base/diff | Base Median (ns) | Diff Median (ns) | Modality|
| -------------------------------------------------------------------------------- | ---------:| ----------------:| ----------------:| --------:|
| System.Text.Json.Serialization.Tests.ReadJson<Dictionary<String, String>>.Deseri | 1.22 | 22031.18 | 18102.24 | |
| System.Text.Json.Serialization.Tests.ReadJson<Dictionary<String, String>>.Deseri | 1.20 | 21554.46 | 17944.26 | |
| System.Text.Json.Serialization.Tests.ReadJson<MyEventsListerViewModel>.Deseriali | 1.20 | 341085.15 | 284277.87 | |
| System.Text.Json.Serialization.Tests.ReadJson<IndexViewModel>.DeserializeFromUtf | 1.19 | 29706.85 | 24903.98 | |
| System.Text.Json.Serialization.Tests.ReadJson<MyEventsListerViewModel>.Deseriali | 1.19 | 330481.64 | 277947.73 | |
| System.Text.Json.Serialization.Tests.ReadJson<MyEventsListerViewModel>.Deseriali | 1.19 | 399397.91 | 336085.63 | |
| System.Text.Json.Serialization.Tests.ReadJson<IndexViewModel>.DeserializeFromStr | 1.18 | 31986.63 | 27064.34 | |
| System.Text.Json.Serialization.Tests.ReadJson<Dictionary<String, String>>.Deseri | 1.18 | 23919.94 | 20268.43 | |
| System.Text.Json.Serialization.Tests.ReadJson<IndexViewModel>.DeserializeFromStr | 1.18 | 31000.14 | 26348.41 | |
| System.Text.Json.Serialization.Tests.ReadJson<LoginViewModel>.DeserializeFromStr | 1.15 | 522.04 | 454.92 | |
| System.Text.Json.Serialization.Tests.ReadJson<Hashtable>.DeserializeFromUtf8Byte | 1.12 | 68993.04 | 61636.12 | |
| System.Text.Json.Serialization.Tests.ReadJson<HashSet<String>>.DeserializeFromUt | 1.10 | 12663.49 | 11477.42 | |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableSortedDictionary<String, | 1.10 | 78887.97 | 71927.75 | |
| System.Text.Json.Serialization.Tests.ReadJson<Hashtable>.DeserializeFromStream | 1.10 | 69968.66 | 63797.29 | |
| System.Text.Json.Serialization.Tests.ReadJson<LoginViewModel>.DeserializeFromStr | 1.09 | 763.93 | 698.34 | |
| System.Text.Json.Serialization.Tests.ReadJson<HashSet<String>>.DeserializeFromSt | 1.09 | 12745.33 | 11734.38 | |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableDictionary<String, String | 1.09 | 48372.57 | 44572.25 | |
| System.Text.Json.Serialization.Tests.ReadJson<ArrayList>.DeserializeFromStream | 1.09 | 51673.60 | 47617.28 | |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableDictionary<String, String | 1.08 | 51226.78 | 47249.65 | |
| System.Text.Json.Serialization.Tests.ReadJson<HashSet<String>>.DeserializeFromSt | 1.08 | 13979.59 | 12973.07 | |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableDictionary<String, String | 1.08 | 48102.61 | 44652.30 | |
| System.Text.Json.Serialization.Tests.ReadJson<BinaryData>.DeserializeFromStream | 1.07 | 807.95 | 755.26 | |
| System.Text.Json.Serialization.Tests.ReadJson<ArrayList>.DeserializeFromString | 1.07 | 49947.46 | 46735.64 | |
| System.Text.Json.Serialization.Tests.ReadJson<BinaryData>.DeserializeFromUtf8Byt | 1.07 | 488.59 | 458.07 | |
| System.Text.Json.Serialization.Tests.ReadJson<LoginViewModel>.DeserializeFromUtf | 1.06 | 426.56 | 400.78 | |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableSortedDictionary<String, | 1.06 | 78155.94 | 73498.13 | |
| System.Text.Json.Serialization.Tests.ReadJson<Hashtable>.DeserializeFromString | 1.06 | 67906.38 | 63896.07 | |
| System.Text.Json.Serialization.Tests.ReadJson<ArrayList>.DeserializeFromUtf8Byte | 1.06 | 50083.93 | 47157.52 | |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableSortedDictionary<String, | 1.05 | 73213.63 | 69703.93 | |
| System.Text.Json.Serialization.Tests.ReadJson<Int32>.DeserializeFromUtf8Bytes | 1.03 | 90.42 | 87.57 | |

benaadams · 2020-08-17T02:44:29Z

Libraries Test Run release coreclr OSX x64 Debug failed: AcceptV4BoundToAnyV6_Success #40913

benaadams · 2020-08-17T03:30:51Z

Note: Avx2 doesn't have less than so arguments are switched for greater than

benaadams · 2020-08-17T03:33:13Z

Note: Also CompareGreaterThan is only sbyte (and CompareLessThan on Sse2), so subtract 0x80 before casting and doing the compare to keep the order correct

benaadams · 2020-08-17T21:52:56Z

Can't workout the linker test issues :(

dotnet build /t:PublishTrimmed /nr:false /warnaserror -p:configuration=Release

/Users/runner/work/1/s/.dotnet/sdk/5.0.100-preview.8.20362.3/NuGet.targets(128,5): Invalid restore input. Invalid target framework 'unsupported'. Input files:
/Users/runner/work/1/s/src/libraries/System.Text.Json/src/System.Text.Json.csproj.
[/Users/runner/work/1/s/artifacts/bin/trimmingTests/projects/Microsoft.Extensions.Logging.Console.TrimmingTests/
AddConsoleFormatterTests/osx-x64/project.csproj] [/Users/runner/work/1/s/src/libraries/Microsoft.Extensions.Logging.Console/tests/TrimmingTests/
Microsoft.Extensions.Logging.Console.TrimmingTests.proj]

layomia · 2020-08-19T18:13:55Z

Taking a look now.

benaadams · 2020-08-19T18:54:49Z

@layomia when I add the browser type $(NetCoreAppCurrent)-Browser to <TargetFrameworks> then the linker tests for TrimmingTests/Microsoft.Extensions.Logging.Console.TrimmingTests.proj which imports System.Text.Json start failing with

Errors
NuGet.targets(128,5): Invalid restore input. Invalid target framework 'unsupported'. Input files: src/libraries/System.Text.Json/src/System.Text.Json.csproj. trimmingTests/projects/Microsoft.Extensions.Logging.Console.TrimmingTests/AddConsoleFormatterTests/linux-x64/project.csproj src/libraries/Microsoft.Extensions.Logging.Console/tests/TrimmingTests/Microsoft.Extensions.Logging.Console.TrimmingTests.proj

On Windows, Linux and OSX. However doesn't seem to provide much helpful detail in the binlog as to why its becoming "unsupported" :(

benaadams · 2020-08-19T19:03:12Z

So the unsupported bit is probably NuGet/Home#5413; which is what NuGet outputs for an unknown framework

benaadams · 2020-08-19T19:26:09Z

Current working idea looking at other build is need to override NuGet.targets with NuGet.RestoreEx.targets somehow

Overriding target "Restore" in project "/__w/1/s/.dotnet/sdk/5.0.100-preview.8.20362.3/NuGet.targets"
with target "Restore" from project "/__w/1/s/.dotnet/sdk/5.0.100-preview.8.20362.3/NuGet.RestoreEx.targets"

layomia

I tried out some changes wrt wasm/non-wasm builds, pls see if this helps - layomia@137ac47. We don't run the linker tests specifically on wasm builds afaict so I don't think any linker test changes are needed in this PR.

layomia · 2020-08-19T17:56:33Z

 <PropertyGroup>
 <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
- <TargetFrameworks>$(NetCoreAppCurrent);netstandard2.0;netcoreapp3.0;net461</TargetFrameworks>
+ <TargetFrameworks>$(NetCoreAppCurrent)-Windows_NT;$(NetCoreAppCurrent)-Unix;$(NetCoreAppCurrent)-Browser;netstandard2.0;netcoreapp3.0;net461</TargetFrameworks>


Why is this change needed?

I think it will only output an AnyCPU assembly if either the TargetFrameworks doesn't have browser or the Architectures aren't included to have wasm which will mean '$(TargetArchitecture)' == 'wasm' will never be true?

benaadams · 2020-08-19T22:49:43Z

We don't run the linker tests specifically on wasm builds afaict so I don't think any linker test changes are needed in this PR.

So its looks like NuGet chokes on <TargetFrameworks>net5.0;net5.0-Browser;...; so the arcade build tools normalize that before restore to <TargetFrameworks>net5.0;... so NuGet doesn't fail.

However the linker tests look to shell out to a basic build which uses the standard restore causing them to fail for all frameworks (where this changed .csproj is included)

benaadams · 2020-08-20T14:59:10Z

🥳

Going to close this PR and open a fresh one as its a bit of a tire fire

benaadams · 2020-08-20T15:49:25Z

Opened #41097

Dotnet-GitSync-Bot added the area-blazor label Aug 15, 2020

benaadams force-pushed the IndexOfOrLessThan branch from efb91b9 to 6cfe3c7Compare August 15, 2020 05:57

benaadams changed the title ~~Intrinsicify JsonReaderHelper.IndexOfOrLessThan~~Intrinsicify JsonReaderHelper.IndexOfOrLessThan [NoMerge]Aug 15, 2020

gfoidl reviewed Aug 15, 2020
View reviewed changes

benaadams force-pushed the IndexOfOrLessThan branch from fecb348 to 1aca492Compare August 15, 2020 13:50

jkotas added area-System.Text.Json and removed area-blazor labels Aug 15, 2020

benaadams force-pushed the IndexOfOrLessThan branch from 1aca492 to 3a58170Compare August 15, 2020 17:00

benaadams marked this pull request as ready for review August 15, 2020 20:30

benaadams requested review from jozkee, layomia and steveharter as code owners August 15, 2020 20:30

benaadams changed the title ~~Intrinsicify JsonReaderHelper.IndexOfOrLessThan [NoMerge]~~Intrinsicify JsonReaderHelper.IndexOfOrLessThanAug 15, 2020

benaadams force-pushed the IndexOfOrLessThan branch from 3a58170 to f4f85a3Compare August 16, 2020 02:42

benaadams mentioned this pull request Aug 17, 2020
[interp] Intrinsify IndexOfOrLessThan #40705
Closed

benaadams commented Aug 17, 2020
View reviewed changes

Comment threadsrc/libraries/System.Text.Json/src/System.Text.Json.csproj Outdated

benaadams mentioned this pull request Aug 17, 2020
CI: System.ArgumentException : The supplied System.Net.Internals.SocketAddress is an invalid size for the System.Net.IPEndPoint end point. #40913
Closed

benaadams commented Aug 17, 2020
View reviewed changes

Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.SpeedOpts.cs Outdated
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.SpeedOpts.cs Outdated

benaadams commented Aug 17, 2020
View reviewed changes

benaadams force-pushed the IndexOfOrLessThan branch 3 times, most recently from 7ffc080 to 3a10197Compare August 17, 2020 16:41

benaadams added 2 commits August 17, 2020 18:54

x86, x64, ARM Intrinsicify JsonReaderHelper.IndexOfOrLessThan
6fb8886

Add optimization for wasm
19facfe

benaadams force-pushed the IndexOfOrLessThan branch from 2108029 to 19facfeCompare August 17, 2020 18:31

Add IncludePlatformAttributes
5635864

try adding framework
c2de3f7

benaadams mentioned this pull request Aug 19, 2020
OSX CI builds failing "Please install openssl before running this script" #41006
Closed

benaadams added 2 commits August 19, 2020 01:54

multi-framework
654a69e

BuildTargetFramework
53a145d

Try including Directory.Build.props
d3f1254

layomia reviewed Aug 19, 2020
View reviewed changes

Feedback
59c60ec

benaadams added 11 commits August 20, 2020 00:56

TargetFramework filter
bd953a7

Output trimming subtask binlog
f13fe7a

Add to Build.props
59dc0d5

paths
b6bcfaa

Import
bc5e9cd

Conditional
d4f1078

path
d4cdcc6

path
51acd99

path
f6b1790

path
53afcd8

Try static restore
541a856

benaadams closed this Aug 20, 2020

benaadams mentioned this pull request Aug 20, 2020
Intrinsicify (Sse, Axv2, Arm64, wasm) JsonReaderHelper.IndexOfOrLessThan #41097
Closed

runfoappBot mentioned this pull request Sep 29, 2020
Test initial triage jaredpar/runfo#50
Closed

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

Uh oh!

Conversation

benaadams commented Aug 15, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gfoidlAug 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benaadams commented Aug 17, 2020

Uh oh!

benaadams commented Aug 17, 2020

Uh oh!

Uh oh!

Uh oh!

benaadamsAug 17, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadamsAug 17, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams commented Aug 17, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

layomia commented Aug 19, 2020

Uh oh!

benaadams commented Aug 19, 2020

Uh oh!

benaadams commented Aug 19, 2020

Uh oh!

benaadams commented Aug 19, 2020

Uh oh!

layomia left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

layomiaAug 19, 2020

Choose a reason for hiding this comment

Uh oh!

benaadamsAug 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benaadams commented Aug 19, 2020

Uh oh!

benaadams commented Aug 20, 2020

Uh oh!

benaadams commented Aug 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benaadams commented Aug 15, 2020•
edited
Loading

benaadamsAug 17, 2020•
edited
Loading

benaadamsAug 17, 2020•
edited
Loading

benaadams commented Aug 17, 2020•
edited
Loading

layomia left a comment •
edited
Loading