Use multi-reg load/store for EncodeToUtf8#95513
Merged
kunalspathak merged 5 commits intodotnet:mainfrom Jan 10, 2024
Merged
Conversation
ghost
added
needs-area-label
Contributor
Author
|
Hi @kunalspathak , this is an initial version of encode to UTF8 using multi-register load/stores. This currently fails some asserts in LSRA phase while doing the crossgen for SPC. It fails while emitting |
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Encoder.cs
Outdated
Show resolved
Hide resolved
Member
|
Does this need an entry in their part notices file? |
Change-Id: Ie56b1786cdf8ac8d2067c0ba1fdfd3924dd9ca13
Contributor
Author
Sorry @danmoseley , I didn't understand which part notices file you're referring to. |
Contributor
Author
|
Initial benchmarking on N1 system show some good performance results. |
a74nh
reviewed
Dec 12, 2023
| } | ||
|
|
||
| end = srcMax - 16; | ||
| if ((Ssse3.IsSupported || AdvSimd.Arm64.IsSupported) && BitConverter.IsLittleEndian && (end >= src)) |
Contributor
There was a problem hiding this comment.
I'm not sure if we should remove the AdvSimd check here.
With this PR, it will use the vector128 version if the buffer length is <48 && >16.
That's probably the best option for speed, but results in a bigger library.
Contributor
|
@SwapnilGaikwad - do you mind sharing the disassembly? |
Contributor
Author
A separately compiled Full assembly |
Contributor
|
Thanks @SwapnilGaikwad for sharing the disassembly. I wanted to see the code quality when consecutive registers are involved. |
SwapnilGaikwad
commented
Dec 13, 2023
| Vector128<byte> res4; | ||
| Vector128<byte> tbl_enc1 = Vector128.Create("ABCDEFGHIJKLMNOP"u8).AsByte(); | ||
| Vector128<byte> tbl_enc2 = Vector128.Create("QRSTUVWXYZabcdef"u8).AsByte(); | ||
| Vector128<byte> tbl_enc3 = Vector128.Create("ghijklmnopqrstuv"u8).AsByte(); |
Contributor
Author
There was a problem hiding this comment.
We can load this encoding table from EncodingMap from Line #774. This could help to reduce the code size but loading from memory/cache would be slightly slower than the regs. Benchmarks didn't show signficant difference. from-mem-artifacts load data from EncodingMap.
| Method | Toolchain | NumberOfBytes | Mean | Error | StdDev | Median | Min | Max | Ratio | MannWhitney(2%) |
|-------------------------------- |-------------------------------------------------------------------------------------- |-------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|---------------- |
| Base64Encode | /main/artifacts/tests/coreclr/linux.arm64.Release/Tests/Core_Root/corerun | 1000 | 363.8 ns | 0.14 ns | 0.12 ns | 363.8 ns | 363.5 ns | 363.9 ns | 1.00 | Base |
| Base64Encode | /runtime/artifacts/tests/coreclr/linux.arm64.Release/Tests/Core_Root/corerun | 1000 | 196.8 ns | 0.02 ns | 0.02 ns | 196.8 ns | 196.8 ns | 196.9 ns | 0.54 | Faster |
| Base64Encode | /runtime/from-mem-artifacts/tests/coreclr/linux.arm64.Release/Tests/Core_Root/corerun | 1000 | 195.3 ns | 0.08 ns | 0.07 ns | 195.3 ns | 195.2 ns | 195.5 ns | 0.54 | Faster |
| | | | | | | | | | | |
| Base64EncodeDestinationTooSmall | /main/artifacts/tests/coreclr/linux.arm64.Release/Tests/Core_Root/corerun | 1000 | 368.2 ns | 0.30 ns | 0.28 ns | 368.2 ns | 367.8 ns | 368.7 ns | 1.00 | Base |
| Base64EncodeDestinationTooSmall | /runtime/artifacts/tests/coreclr/linux.arm64.Release/Tests/Core_Root/corerun | 1000 | 191.5 ns | 0.04 ns | 0.04 ns | 191.5 ns | 191.5 ns | 191.6 ns | 0.52 | Faster |
| Base64EncodeDestinationTooSmall | /runtime/from-mem-artifacts/tests/coreclr/linux.arm64.Release/Tests/Core_Root/corerun | 1000 | 194.0 ns | 0.22 ns | 0.20 ns | 193.9 ns | 193.7 ns | 194.4 ns | 0.53 | Faster |
Would you suggest to read encoding table from EncodingMap?
Contributor
|
/azp run runtime-coreclr libraries-jitstress |
Contributor
|
/azp run runtime-coreclr libraries-jitstress2-jitstressregs |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
|
/azp run runtime-coreclr libraries-jitstressregs |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
I guess he is talking about the reference made in https://github.com/dotnet/runtime/pull/95513/files#diff-b3b9edcf4c0d62e78954d826c44005cffb306b6ccf155f1a9228669229b7e765R496, but not sure where exactly to add this. @danmoseley - can you please confirm? |
teo-tsirpanis
added
area-System.Buffers
and removed
needs-area-label
|
Tagging subscribers to this area: @dotnet/area-system-buffers Issue DetailsThis implements the encode to UTF8 algorithm here.
|
Contributor
|
ping @danmoseley |
Member
|
Oops, yes, that was what caught my eye. Generally if we use significant ideas/code from elsewhere we add a credit in THIRD-PARTY-NOTICES.TXT at the root. Up to you. |
Contributor
Looks like there is already an entry in there, but it's not immediately obvious. runtime/THIRD-PARTY-NOTICES.TXT Line 345 in 57bcccd Contains an extra copy of the license from https://github.com/aklomp/base64/blob/master/LICENSE |
kunalspathak
approved these changes
Jan 9, 2024
| [CompExactlyDependsOn(typeof(AdvSimd.Arm64))] | ||
| private static unsafe void AdvSimdEncode(ref byte* srcBytes, ref byte* destBytes, byte* srcEnd, int sourceLength, int destLength, byte* srcStart, byte* destStart) | ||
| { | ||
| // C# implementatino of https://github.com/aklomp/base64/blob/3a5add8652076612a8407627a42c768736a4263f/lib/arch/neon64/enc_loop.c |
Contributor
There was a problem hiding this comment.
nit
Suggested change
| // C# implementatino of https://github.com/aklomp/base64/blob/3a5add8652076612a8407627a42c768736a4263f/lib/arch/neon64/enc_loop.c | |
| // C# implementation of https://github.com/aklomp/base64/blob/3a5add8652076612a8407627a42c768736a4263f/lib/arch/neon64/enc_loop.c |
Contributor
There was a problem hiding this comment.
you can do this in a follow-up PR
This was referenced Jan 9, 2024
kunalspathak
added a commit
to kunalspathak/runtime
that referenced
this pull request
Jan 13, 2024
This reverts commit fdb03ca.
tmds
pushed a commit
to tmds/runtime
that referenced
this pull request
Jan 23, 2024
* Use multi-reg load/store for EncodeToUtf8 * Use the fixed version of multi-reg store * Update variable naming
tmds
pushed a commit
to tmds/runtime
that referenced
this pull request
Jan 23, 2024
…#96944) * Revert "[libs] Skip AdvSimdEncode on Mono (dotnet#96829)" This reverts commit 1a76e37. * Revert "Use multi-reg load/store for EncodeToUtf8 (dotnet#95513)" This reverts commit fdb03ca. * Wrap load/store vector APIs in '#if false' * Disable load/store vector tests * remove the trailing space
Member
|
I assume this is the API being discussed. If so, it would be good to put in the initial comment so that it is easy for folks to fine. https://learn.microsoft.com/dotnet/api/system.buffers.text.base64.encodetoutf8 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This implements the encode to UTF8 algorithm here.