Skip to content
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

Spanified Webencoders.Base64UrlEncode #11047

Merged
merged 5 commits into from
Jun 11, 2019

Conversation

gfoidl
Copy link
Member

@gfoidl gfoidl commented Jun 10, 2019

Added string Base64UrlEncode(ReadOnlySpan<byte> input) to WebEncoders, and
redirected the other string Base64UrlEncode(...)-methods to use the spanified version.

Used this new method in SignalR's HttpConnectionManager.MakeNewConnectionId.

So in total it safes two allocations. One in MakeNewConnectionId, as the buffer is stackalloced, the other one in the base64 encoding, as the buffer is stackalloced or rented from the ArrayPool.

Addresses dotnet/extensions#456

Note:
This fix just safes the allocations. In this PR no attempt was made to do base64Url
encoding directly -- i.e. as O(n) operation instead a O(2n) as used here, where in the
base64 encoded results + and / are substituted (see dotnet/extensions#338 for a (declined) PR that does this directly).
Further no attempt was made to vectorize the encoding (similar to dotnet/corefx#34529, or as used in https://github.com/gfoidl/Base64).
Perf-improvments will come for free once dotnet/coreclr#21833 is merged.

/CC: @BrennanConroy

@BrennanConroy
Copy link
Member

To resolve the code check failure, run dotnet msbuild /t:GenerateReferenceSource in the "src\Http\WebUtilities\src" directory.

Also, there look to be some compile errors.

@gfoidl
Copy link
Member Author

gfoidl commented Jun 10, 2019

in the "src\Http\WebUtilities\src" directory.

Ah, didn't realize the usage over there. Thx.

some compile errors.

E.g.

/home/vsts/work/1/s/src/Shared/WebEncoders/WebEncoders.cs(213,40): error CS1503: Argument 1: cannot convert from 'System.Span<char>' to 'char*' [/home/vsts/work/1/s/src/Shared/test/Shared.Tests/Microsoft.AspNetCore.Shared.Tests.csproj]
/home/vsts/work/1/s/src/Shared/WebEncoders/WebEncoders.cs(333,21): error CS0117: 'Convert' does not contain a definition for 'TryToBase64Chars' [/home/vsts/work/1/s/src/Shared/test/Shared.Tests/Microsoft.AspNetCore.Shared.Tests.csproj]

I believed it's .NET Core 3.0 only...proved wrong 😢
https://github.com/aspnet/AspNetCore/blob/0b340bd70cfe41200da593c08f4a88c6fe9efa74/src/Shared/test/Shared.Tests/Microsoft.AspNetCore.Shared.Tests.csproj#L4

Will fix this in the next commit (Edit: in the next one...)

@gfoidl gfoidl requested review from jkotalik and Tratcher as code owners June 10, 2019 19:41
@gfoidl gfoidl force-pushed the webencoders-span branch from f8db6c6 to 64db801 Compare June 10, 2019 20:06
@gfoidl
Copy link
Member Author

gfoidl commented Jun 10, 2019

The code-check fails. Will generate the ref-assembly again tomorrow. There's same strange in my local setup with not found target-frameworks, etc (I followed the steps documented in this repo, so I assume it's me doing something wrong).

@gfoidl
Copy link
Member Author

gfoidl commented Jun 10, 2019

The code-check fails

Gave it another try. Failed it before because I moved the method down in 64db801?

By executing the command from #11047 (comment) I get (linux-box):

Microsoft (R) Build Engine version 16.2.0-preview-19274-01+7708b265e for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

  Microsoft.Net.Http.Headers -> /home/gfoidl/test/AspNetCore/artifacts/bin/Microsoft.Net.Http.Headers/Debug/netcoreapp3.0/Microsoft.Net.Http.Headers.dll
  Microsoft.AspNetCore.WebUtilities -> /home/gfoidl/test/AspNetCore/artifacts/bin/Microsoft.AspNetCore.WebUtilities/Debug/netcoreapp3.0/Microsoft.AspNetCore.WebUtilities.dll
  /bin/sh: 8: /tmp/tmp0888b99f43f74abebedee29767dea158.exec.cmd: TargetFramework: not found

Is this known / expected?

@BrennanConroy
Copy link
Member

@aspnet/build Does GenerateReferenceSource not work on Linux if net461 is a tfm?

@analogrelay analogrelay added the area-signalr Includes: SignalR clients and servers label Jun 10, 2019
@analogrelay analogrelay removed their request for review June 10, 2019 23:47
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super happy 😄

@natemcmaster
Copy link
Contributor

I don't think GenerateReferenceSource works on Linux at all due to MSBuild not handling line endings and slashes properly.

@BrennanConroy BrennanConroy merged commit 90ab2cb into dotnet:master Jun 11, 2019
@BrennanConroy
Copy link
Member

Thanks @gfoidl !

@gfoidl gfoidl deleted the webencoders-span branch June 11, 2019 05:43
@gfoidl
Copy link
Member Author

gfoidl commented Jun 11, 2019

@natemcmaster on linux the ref-assembly was generated correctly, but the command showed the above failure. The ref-assembly is for netcoreapp3.0 only, maybe that's why it worked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-signalr Includes: SignalR clients and servers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants