-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Base64url encoding/decoding #102364
Add Base64url encoding/decoding #102364
Conversation
Note regarding the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just skimmed over it...
I saw your comment, but for checking perf some of my comments may help (a bit).
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Decoder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Url/Base64UrlDecoder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Url/Base64UrlDecoder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Url/Base64UrlEncoder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Url/Base64UrlEncoder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Url/Base64UrlEncoder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Url/Base64UrlValidator.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Validator.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Günther Foidl <[email protected]>
/benchmark |
Thank you, sure perf related feedbacks appreciated, I updated the description, any feedbacks are welcome. |
Crank Pull Request Bot
Benchmarks:
Profiles:
Components:
Arguments: any additional arguments to pass through to crank, e.g. |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Show resolved
Hide resolved
Co-authored-by: Jeremy Barton <[email protected]>
padding++; | ||
} | ||
|
||
if (TBase64Decoder.IsValidPadding(Unsafe.Subtract(ref ptrToLastElement, 1))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What guarantees this is in bounds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only called with a buffer that has size of 4
runtime/src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Decoder.cs
Lines 498 to 499 in e617b8f
const int BlockSize = 4; | |
Span<byte> buffer = stackalloc byte[BlockSize]; |
runtime/src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Decoder.cs
Line 545 in e617b8f
int paddingCount = GetPaddingCount<TBase64Decoder>(ref buffer[^1]); |
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Decoder.cs
Outdated
Show resolved
Hide resolved
|
||
if (src == srcEnd) | ||
goto DoneExit; | ||
} | ||
|
||
end = srcMax - 64; | ||
end = srcMax - 32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this change from 64 to 32?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was 32 originally, but unintentionally changed with this PR
The method comment also mentions it require 32 byte
runtime/src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Encoder.cs
Lines 316 to 322 in e617b8f
private static unsafe void Avx2Encode<TBase64Encoder, T>(ref byte* srcBytes, ref T* destBytes, byte* srcEnd, int sourceLength, int destLength, byte* srcStart, T* destStart) | |
where TBase64Encoder : IBase64Encoder<T> | |
where T : unmanaged | |
{ | |
// If we have AVX2 support, pick off 24 bytes at a time for as long as we can. | |
// But because we read 32 bytes at a time, ensure we have enough room to do a | |
// full 32-byte read without segfaulting. |
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Url/Base64UrlValidator.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Decoder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Decoder.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Decoder.cs
Outdated
Show resolved
Hide resolved
{ | ||
const int BlockSize = 4; | ||
int BlockSize = Math.Min(source.Length - (int)sourceIndex, 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would anything bad happen if this were left at a const 4
?
This not being const would penalize the non-url path as well a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Base64Url
when the source is not multiple of 4 we need to adjust the buffer size accordingly, the BlockSize
value used to fill the whitespace with padding (row 653) so that it could decoded correctly (in case remaining bytes were decodable or valid)
@MihuBot fuzz Base64 |
also showing regressions in dotnet/perf-autofiling-issues#36512 and improvements dotnet/perf-autofiling-issues#36643 |
Base64Url encoding doesn't append padding, therefore encoded byte count differs from Base64 encoding:
Base64Url decoding ignore whitespace and padding, therefore decodable byte count differs from Base64 decoding, the exact decoding result also depend on
isFinalBlock
and if padding involvedApproved API shape:
Draft PR until below list is completed, for now mainly for checking perf, run tests on different CI legs, and get early feedback.
Fixes #1658