-
Notifications
You must be signed in to change notification settings - Fork 763
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
Added Span support to Webencoders with direct encoding #338
Conversation
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.
Notes for review
@@ -96,144 +172,185 @@ public static byte[] Base64UrlDecode(string input, int offset, int count) | |||
/// </remarks> | |||
public static byte[] Base64UrlDecode(string input, int offset, char[] buffer, int bufferOffset, int count) |
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.
buffer
isn't needed, because there is no intermediate step input -> base64
(which would be in buffer
).
This is OK according to the xml comment ("Content is not preserved.").
So the checks with / for buffer
could be eliminated?!
Rebased due to conflict in |
Seems to be related to dotnet/corefxlab#1808 |
@gfoidl we didn't forget this. We should look at it early for 2.2 (right now). |
Rebased due to
|
The <StandardTestTfms> is netcoreapp2.2 + net461 (on win), but as the file will be added as source to "consumers" other target frameworks are masked in the source. net461 as target for desktop. netcoreapp2.0 so ArrayPool can be used. netcoreapp2.1 for string.Create netcoreapp2.2 for the current tfm (no other enhanced features used over netcoreapp2.1).
@@ -251,7 +251,8 @@ public static unsafe string Base64UrlEncode(ReadOnlySpan<byte> data) | |||
|
|||
var base64Len = GetBufferSizeRequiredToBase64Encode(data.Length, out int numPaddingChars); | |||
var base64UrlLen = base64Len - numPaddingChars; | |||
#if NETCOREAPP2_1 | |||
|
|||
#if NETCOREAPP2_2 || NETCOREAPP2_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.
With dotnet/sdk#1813 this could be written cleaner.
@gfoidl can you clean up the inline if defs? Maybe splitting the files would be better and clearer to read, |
You mean something like I would just duplicate the methods, and attach the if def around them, so it's easiert to read. OK? |
@pakrym, @davidfowl this seems to be still waiting for your reviews. |
@gfoidl Sorry I dropped the ball on this one, after much consideration we won't be taking it, but we'll be taking the other improvements to StringSegment. Thanks again and very sorry for the lack of attention here. |
@davidfowl no problem, that's part of the game 😉 I'd like to say thank you too, as I learned quite a lot with these PRs -- escepially for the then new type Span. |
As info if anyone seeks a base64Url encoder / decoder and lands here: in https://github.com/gfoidl/Base64 I've made a library for this and put it on steroids (aka simd). Right now it's in "preview" state, as hardware intrinsics in .NET Core 3.0 are also in preview state. |
@gfoidl nice! cc @GrabYourPitchforks |
Reference `Entropy` localization samples
Description
This PR is -- at least for me -- a better solution for https://github.com/aspnet/Home/issues/2966 than #334.
It is built on top of #334 (these commits are included in this PR, but squashed).
In #334 I didn't like the route
data -> base64 -> url-chars-substitution
, because it could be done in a direct waydata -> base64Url
. That PR was done that way, because the existing code was based on that route, and I interpreted https://github.com/aspnet/Home/issues/2966 that way.I found some time to try the direct way, the results seem very good, and the code is cleaner.
The url chars substitution produced quite a lot of code, and encoding / decoding is done in
O(2n)
.In this PR the encoding / decoding is done in
O(n)
, and no intermediate buffers are needed.The encoding / decoding code is based on
Benchmarks
Results from Benchmark-Project
Baseline
See results in #334
This PR
Individual benchmarks
Project for this benchmarks
Decode Guid
Decode Data
Encode Guid
Encode Data