Vectorize IA5 and Visible strings in ASN.1 encoding and decoding.#131109
Conversation
This vectorizes the IA5Encoding and VisibleStringEncoding for ASN.1 encoding and decoding.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
/cc @tannergooding since this is adjacent to your wheelhouse. |
|
cc @dotnet/area-system-formats-asn1 |
|
Tagging subscribers to this area: @dotnet/area-system-formats-asn1, @bartonjs, @vcsjones |
There was a problem hiding this comment.
Pull request overview
This PR improves System.Formats.Asn1 character-string handling by introducing a vectorized fast-path for IA5String and VisibleString encoding/decoding on .NET (while keeping downlevel behavior unchanged), and extends tests to cover vector-width boundaries and invalid-data index reporting.
Changes:
- Introduces
RestrictedAsciiRangeEncodingfor.NETCoreAppbuilds, usingSystem.Numerics.Vector<T>to validate/convert in vector-sized chunks with scalar fallback to preserve exception-index behavior. - Switches
IA5EncodingandVisibleStringEncodingto derive fromRestrictedAsciiRangeEncoding(range-based validation) instead of the existing allowed-table implementation. - Adds targeted reader/writer tests for vector-sized payloads and “invalid after vector prefix” scenarios.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnCharacterStringEncodings.net.cs | Adds vectorized range-based encoding/decoding implementation for IA5/VisibleString on .NET. |
| src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnCharacterStringEncodings.downlevel.cs | Keeps downlevel behavior by mapping RestrictedAsciiRangeEncoding to the existing restricted ASCII encoder. |
| src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnCharacterStringEncodings.cs | Updates IA5/VisibleString encodings to use the new range-based base type. |
| src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj | Adds conditional compile includes for the new files and references System.Numerics.Vectors for .NET builds. |
| src/libraries/System.Formats.Asn1/tests/Writer/WriteIA5String.cs | Adds an invalid IA5 input with an invalid character after a vector-sized prefix. |
| src/libraries/System.Formats.Asn1/tests/Writer/SimpleWriterTests.cs | Adds VisibleString writer tests for vector-sized payloads and invalid character index reporting. |
| src/libraries/System.Formats.Asn1/tests/Reader/ReadIA5String.cs | Adds IA5 reader coverage for a longer valid input and a vector-prefix invalid input case. |
| src/libraries/System.Formats.Asn1/tests/Reader/ComprehensiveReadTests.cs | Adds VisibleString reader tests for vector-sized payloads and invalid byte index reporting. |
Copilot's findings
- Files reviewed: 8/8 changed files
- Comments generated: 1
|
Should be able to get to this later tonight. At a high level glance it generally looks good/correct and following all the guidance. I notably did just get a rewrite of the public SIMD (on docs.microsoft.com) done and have a PR up adding a vectorization skill for review/authoring, so if you're up for it then it might be a good first trial of it (not merged yet): #131108 |
|
Generally looks good to me, nothing stood out in review (though I didn't stop and think hard about the bounds). Have the benchmarks been run against strings at various offsets, just making sure alignment isn't a problem. Since one of the tests clearly is operating at alignment+3, I'm going to guess it's all good, but it still feels like something worth checking. |
Seems fine and still significant performance gains. Results (too big for a GH comment): https://gist.github.com/vcsjones/6500b884c3f145f0c3fb11749dbf566c |
tannergooding
left a comment
There was a problem hiding this comment.
LGTM.
I also tested this locally against the new vectorization skill and depending on model the feedback was generally along the lines of:
- You might be able to add a fast path to reuse
Ascii.IsValidfor the cases the range is "all ascii" - This is using
Vector<T>rather than the fixed-width vectors and so may leave perf on the table - No explicit tests using
BoundedMemorywere added to help ensure safety
None of those I view as "blocking", particularly given the area and the way the code is setup.
I think the code reuse between IA5String and VisibleString is more valuable. Neither I don't think are quite used enough to warrant intense optimization to eke out every ounce of performance.
My impression was that |
It should be, especially if you use the Like I said, I didn't personally view any of that as blocking, more just "automatic callouts" that vectorization PRs may get from Copilot in the future (which will likely always be a bit stricter than a human reviewer). |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnCharacterStringEncodings.net.cs:154
- Same as above: the comment says it returns the position "before the current vector's width", but the code returns the current position. Please align the comment with the actual behavior.
// If any element in the vector is not allowed, we break out and return the position before the
// current vector's width so that it goes down the scalar path. The scalar path will determine the
// precise location of the invalid element.
- Files reviewed: 8/8 changed files
- Comments generated: 1
This vectorizes the IA5Encoding and VisibleStringEncoding for ASN.1 encoding and decoding for the .NET implementations. Downlevel remains unchanged. Validation, encoding, and decoding will now operate on length of
Vector<T>at a time instead of processing each input character-by-character or byte-by-byte.Overall this shows good performance improvements. Allocations remain unchanged, and through improves for typical inputs. This only uses high-level
System.NumericsVector<T>- using intrinsics was not a goal of this change. This ensure we don't need to account for minor platform behavior differences.For a 32 element input, decoding goes from 51.6ns to 17.7ns. Writing goes from 52.76ns to 21.06ns. Full benchmarks are available below the fold. Benchmark code is available at https://gist.github.com/vcsjones/73dbd0ae386b62a68b4d03cf71ed933d.
Error paths may be slightly slower. This is because we process data one vector of length at a time, including validation. However in the event of invalid input, we have to back up and process by scalar to find the exact position where the invalid element is to maintain exception compatibility. This feels like a worthy trade off, both because it's an error path and the exception throw and unwinding is still going to dominate, and for simplicity.
Some test cases were added to ensure all validation paths are correctly hit.
Performance