Extend WebEncoders API to avoid allocations within the methods
#563
Conversation
- #23 part 4 - depends on aspnet/HttpAbstractions#563 nits: - correct name of a field in `AntiforgerySerializationContext` - avoid allocations when returning an `AntiforgerySerializationContext` in (unlikely) case `Stream` is unused
| // If the caller provided invalid base64 chars, they'll be caught here. | ||
| return Convert.FromBase64CharArray(completeBase64Array, 0, completeBase64Array.Length); | ||
| return Convert.FromBase64CharArray(input, offset, arraySizeRequired); |
There was a problem hiding this comment.
Yes, this call allocates. Convert doesn't offer a way around it.
|
Why are you making this change? |
Because the allocations done inside |
|
Seems like it might be worthwhile to consider a low allocation pattern where the user provides all of the input and output structures. They way they can pool arrays or instead of us having to new things up. The only issue might be knowing what the size of the output array is. I wonder of there are any nice patterns for doing this. |
- rewrite existing methods in terms of the new ones - don't allocate multiple 0-length arrays nit: - clarify a couple of doc comments e.g. using `<paramref/>` - move an error message into a resource - pass parameter names into new resource - rename parameters for consistency e.g. `inputLength` -> `count`
Also name literal `int` parameters.
dougbu
force-pushed
the
dougbu/expose.arrays
branch
from
881e22e
to
d0c8adc
Compare
- #23 part 4 - depends on aspnet/HttpAbstractions#563 nits: - correct name of a field in `AntiforgerySerializationContext` - avoid allocations when returning an `AntiforgerySerializationContext` in (unlikely) case `Stream` is unused
Also name literal `int` parameters.
|
🆙📅 |
| /// <returns> | ||
| /// The number of characters written to <paramref name="output"/>, less any padding characters. | ||
| /// </returns> | ||
| public static int Base64UrlEncode(byte[] input, int offset, int count, char[] output, int outputOffset) |
There was a problem hiding this comment.
By convention, the count in these methods always comes last (Buffer.BlockCopy, string.Compare)
There was a problem hiding this comment.
Not always and I followed what Convert.ToBase64CharArray() exposes. See https://github.com/aspnet/HttpAbstractions/pull/563/files#diff-3fdfb7eef02148bf1131ee1c77af1fe6R282 But I don't feel strongly. Do you?
There was a problem hiding this comment.
Thought about this a bit more, Convert.ToBase64CharArray() is the odd one out. I'll move count to the end here and in the new Base64UrlDecode() overload.
|
Another option here is to use |
from me with some minor comments - this will meet our needs.- more `var` - move `count` parameters to the end Also added a test covering the new overloads.
|
BTW @rynowak
This option is now open for callers. For example, if the DataProtections use of |
nit:
<paramref/>inputLength->count