-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Add Span<T> Base64 conversion APIs that support UTF-8 #24888
Conversation
It would be more fair to use the non-allocating variant. |
I couldn't find any non-allocating public API for decoding (https://msdn.microsoft.com/en-us/library/system.convert_methods(v=vs.110).aspx). Both of these return a
|
The current .NET Core has TryFromBase64Chars |
I updated the performance test. Although there wasn't much difference in the results. |
Then can you also fix the baseline? We shouldn't be happy about such a difference. We should be happy that we've found ways to improve the existing one and then do it. I can believe there could be a small difference due to fundamental things like outputting 1 byte vs 2 bytes per encoded unit, but it shouldn't be anywhere near that big. Success isn't making the new thing faster. Success is making the new thing fast and then making the existing one as fast or as close to as fast as is possible. |
int destIndex = 0; | ||
int result = 0; | ||
|
||
if (destLength >= GetMaxEncodedToUtf8Length(srcLength)) |
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 will throw when the source is too big. Should it rather return OperationStatus.DestinationTooSmall
?
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.
I considered that. The issue with DestinationTooSmall is the user will then expect to enlarge the output buffer and try again, and for any buffer size up to int.MaxValue, they won't succeed for the current input length.
However, if they could continue to slice the input as it is partially consumed, then they would eventually succeed (or they could get unmanaged memory using AllocHGlobal). So, I will update the behavior as per your suggestion.
var encodedLength = GetMaxEncodedToUtf8Length(dataLength); | ||
if (buffer.Length < encodedLength) goto FalseExit; | ||
|
||
var leftover = dataLength - dataLength / 3 * 3; // how many bytes after packs of 3 |
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.
Nit: It is not ok to use var here per corefx coding conventions.
|
||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
private static int Encode(ref byte threeBytes) | ||
{ |
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.
You should be able to use same set of tricks here as you are using for Decode: cache the map in a local upfront, avoid bound checks for it and save some binary ops by operating on the whole byte. Something like:
private static int Encode(ref byte threeBytes, ref byte encodingMap)
{
int i = (threeBytes << 16) | (Unsafe.Add(ref threeBytes, 1) << 8) | Unsafe.Add(ref threeBytes, 2);
int i0 = Unsafe.Add(ref encodingMap, i >> 18)
int i1 = Unsafe.Add(ref encodingMap, (i >> 12) & 0x3F);
int i2 = Unsafe.Add(ref encodingMap, (i >> 6) & 0x3F);
int i3 = Unsafe.Add(ref encodingMap, i & 0x3F);
return i0 | (i1 << 8) | (i2 << 16) | (i3 << 24);
}
52, 53, 54, 55, 56, 57, 43, 47 //4..9, +, / | ||
}; | ||
|
||
const byte s_encodingPad = (byte)'='; // '=', for padding |
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.
Nit: s_
is not correct prefix for consts per corefx coding conventions
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
public static int GetMaxDecodedFromUtf8Length(int length) | ||
{ | ||
Debug.Assert(length >= 0); |
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.
Do we like debug assert here? Should we return 0 or throw?
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
public static int GetMaxEncodedToUtf8Length(int length) | ||
{ | ||
Debug.Assert(length >= 0); |
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.
Same, i.e. I think we should throw for negative lengths. If you do decide to throw, throw from a helper to allow inlining.
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.
Agreed. Public members need to either accept their inputs (no assert, no check) or properly validate (check and throw). Asserting on inputs is only valid for non-public members, to guard that the public member already did the validation.
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.
Do we care what exception type gets thrown? Personally I don't care if it's ArgumentException or OverflowException, or even if it's potentially both (negative vs. too large). It's not like the developer can catch and handle it anyway.
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.
ArgumentOutOfRangeException seems to match the problem space to me. (Well, really, argument out of domain (since it's an input); but that's just my math degree talking)
I agree. Filed an issue to look into this. Ty. |
@@ -106,12 +106,12 @@ public static T ReadMachineEndian<T>(ReadOnlySpan<byte> buffer) | |||
#else | |||
if (RuntimeHelpers.IsReferenceOrContainsReferences<T>()) | |||
{ | |||
throw new ArgumentException(SR.Format(SR.Argument_InvalidTypeWithPointersNotSupported, typeof(T))); | |||
ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(T)); |
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 is the reason for this change?
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.
From @KrzysztofCwalina
If you do decide to throw, throw from a helper to allow inlining.
Previously, ThrowHelper.cs was not included for netcoreapp, but I updated the project file to always include ThrowHelper.cs. It is the same exception and message.
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.
The changes in this file should have zero effect on the inlining or the native code that will be generated. I do not see a problem with, but I do not think they help or hurt anything.
|
||
ref byte encodingMap = ref s_encodingMap[0]; | ||
|
||
if (srcLength <= MaximumEncodeLength && destLength >= GetMaxEncodedToUtf8Length(srcLength)) |
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 it make sense to cap the srcLength to whatever fits instead of having two copies of the encoding loop? It would avoid the penalty for the case where the destination buffer does not fit, and make the code quite a bit smaller (note that you get pretty non-trivial code expansion here because of the use of the AggressiveInlining attributes).
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.
Same in the decoder.
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.
Great idea. I was tinkering with that but was previously struggling to get the "cap" set correctly. But I believe I have figured it out now!
} | ||
|
||
// Pre-computing this table using a custom string(s_characters) and GenerateDecodingMapAndVerify (found in tests) | ||
static readonly sbyte[] s_decodingMap = { |
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.
private
. corefx coding style is to be explicit with visibility.
} | ||
|
||
// Pre-computing this table using a custom string(s_characters) and GenerateEncodingMapAndVerify (found in tests) | ||
static readonly byte[] s_encodingMap = { |
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.
private
@dotnet-bot test OSX x64 Debug Build |
* Add Span<T> Base64 conversion APIs that support UTF-8. * Optimize the encoding loop when there is plenty of available space * Optimize EncodeInPlace and update DecodeBaseline perf test. * Addressing PR feedback, encode optimization, throw for negative lengths * Reenable commented out perf tests. * Cap the amount of data to process based on how much that will fit. * Being explicit with access modifiers to follow guidelines.
…24888) * Add Span<T> Base64 conversion APIs that support UTF-8. * Optimize the encoding loop when there is plenty of available space * Optimize EncodeInPlace and update DecodeBaseline perf test. * Addressing PR feedback, encode optimization, throw for negative lengths * Reenable commented out perf tests. * Cap the amount of data to process based on how much that will fit. * Being explicit with access modifiers to follow guidelines. Commit migrated from dotnet/corefx@b7b3439
Fixes https://github.com/dotnet/corefx/issues/24568 (part of https://github.com/dotnet/corefx/issues/24174).
Note:
OperationStatus (previously TransformationStatus) can be updated base on API review and doesn't need to block this PR (https://github.com/dotnet/corefx/issues/22412).
Code Coverage:
100% line and branch coverage
Performance Test Results (duration):
Out-dated performance results 1
Out-dated performance results 2
Out-dated performance results 3
Encode is about 10% faster
20% slower10% slower (for input larger than 100 bytes). Decode is at least 4x faster (even when compared to the new non-allocating variant TryFromBase64Chars).This is probably because the Convert.FromBase64CharArray allocates (approximately n bytes, where n is input size + around 24 byte overhead).Edit 0: Made encode a bit faster for the common case.
Edit 1: Compared against TryFromBase64Chars, there is still a large perf difference.
Edit 2: Added new performance numbers based on additional optimizations suggested by @jkotas.
Edit 3: Added performance tests for when destination buffer is too small.
cc @KrzysztofCwalina, @stephentoub, @GrabYourPitchforks, @jkotas, @dotnet/corefxlab-contrib