Use NatsMemoryOwner for Base64Url Encoder#565
Conversation
to11mtm
left a comment
to11mtm
left a comment
There was a problem hiding this comment.
Self review with asks for feedback.
| { | ||
| using (var owner = EncodeToMemoryOwner(inArray, raw)) | ||
| { | ||
| var segment = owner.DangerousGetArray(); |
There was a problem hiding this comment.
The use here isn't dangerous since we are owning it in scope,
This was just easiest way to call new string without #if type stuff for different FWs.
As it stands, this should still be a significant improvement as far as array allocations due to the pooling.
LMK if this deserves a comment.
There was a problem hiding this comment.
I think it's good. as you said it's in scope so it's fine. a short comment might be nice for our future selves. btw if you're concerned maybe use span.tostring()? - i believe that was you suggestion to something else before 😅 having said that I'm guessing string.ctor(char[]) must be super fast!
There was a problem hiding this comment.
span.tostring()?
I had the same realization lol... span.ToString is going to be the best way to go on these.
| var lengthMod3 = length % 3; | ||
| var limit = length - lengthMod3; | ||
| var output = new char[(length + 2) / 3 * 4]; | ||
| var owner = NatsMemoryOwner<char>.Allocate((length + 2) / 3 * 4); |
There was a problem hiding this comment.
Should we be try-catch-dispose-throw around this, case something derps in the encode?
There was a problem hiding this comment.
Also, Should we be adding a flag to the method here to specify 'clearing' and pass it in here? I guess it depends on the security concern of the SHA lingering.
CC @mtmk @caleblloyd
There was a problem hiding this comment.
Should we be try-catch-dispose-throw around this, case something derps in the encode?
Should we encode to span instead? in case of sha we can also stackalloc maybe?
Also, Should we be adding a flag to the method here to specify 'clearing' and pass it in here?
Do you mean clearing the input array or output?
There was a problem hiding this comment.
Should we encode to span instead? in case of sha we can also stackalloc maybe?
Hmm I can give it a shot, it might be cleaner...
Do you mean clearing the input array or output?
Clearing the buffered array. IIRC NatsMemoryOwner has a flag to say whether the array is cleared on return.
My 'gut' says yes but if we can get away with stackallocing the span, point is moot. Will check.
| if (info.Digest == null | ||
| || info.Digest.StartsWith("SHA-256=") == false | ||
| || info.Digest.AsSpan().Slice("SHA-256=".Length).SequenceEqual(digest.Span) == false) |
There was a problem hiding this comment.
There is likely a 'better way' to handle this case, but this seemed clear and should be performant enough.
There was a problem hiding this comment.
I think it's good. maybe ordinal StartsWith to avoid cultures?
2 participants
Resolves #557
By using NatsMemoryOwner we can avoid a lot of GC thrashing here.
Note:
Once Microsoft.Bcl.Memory hits 9.0, there will be a
Base64Urlclass to get rid of our encoder, see : dotnet/runtime#103617If we are OK pulling in 'odd-numbered' deps, once that is released, we will want to revisit the topic.
Above note aside, this PR should lower GC pressure on ObjectStore operations all targets (i.e. NETSTANDARD/NET).
I kept it as simple as I could in the mindset that longer term, we can lean on BCL instead, i.e. Future PRs can optimize for scenarios where more IFDEFs are worth the cost and/or we update deps.