Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Add Base64url encoding/decoding#102364

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
buyaa-n merged 40 commits intodotnet:mainfrombuyaa-n:base64url
Jun 14, 2024
Merged

Add Base64url encoding/decoding#102364

buyaa-n merged 40 commits intodotnet:mainfrombuyaa-n:base64url
Jun 14, 2024

Conversation

buyaa-n
Copy link
Contributor

@buyaa-nbuyaa-n commentedMay 17, 2024•
edited
Loading

  • Base64Url encoding doesn't append padding, therefore encoded byte count differs from Base64 encoding:

    Source byte countBase64 encoded byte countBase64Url encoded byte count
    142
    243
    344
    486
    587
    688

  • Base64Url decoding ignore whitespace and padding, therefore decodable byte count differs from Base64 decoding, the exact decoding result also depend onisFinalBlock and if padding involved

    Source byte countBase64 decoded byte countBase64Url decoded byte count
    1max 0max 0
    2max 0max 1
    3max 0max 2
    4max 3max 3
    5max 3max 3
    6max 3max 4
    7max 3max 5
    8max 6max 6

Approved API shape:

namespaceSystem.Buffers.Text;publicstaticclassBase64Url{publicstaticintGetMaxDecodedLength(intbase64Length);publicstaticintGetEncodedLength(intbytesLength);publicstaticOperationStatusEncodeToUtf8(ReadOnlySpan<byte>source,Span<byte>destination,outintbytesConsumed,outintbytesWritten,boolisFinalBlock=true);publicstaticintEncodeToUtf8(ReadOnlySpan<byte>source,Span<byte>destination);publicstaticboolTryEncodeToUtf8(ReadOnlySpan<byte>source,Span<byte>destination,outintcharsWritten);publicstaticbyte[]EncodeToUtf8(ReadOnlySpan<byte>source);publicstaticOperationStatusEncodeToChars(ReadOnlySpan<byte>source,Span<char>destination,outintbytesConsumed,outintcharsWritten,boolisFinalBlock=true);publicstaticintEncodeToChars(ReadOnlySpan<byte>source,Span<char>destination);publicstaticboolTryEncodeToChars(ReadOnlySpan<byte>source,Span<char>destination,outintcharsWritten);publicstaticchar[]EncodeToChars(ReadOnlySpan<byte>source);publicstaticstringEncodeToString(ReadOnlySpan<byte>source);publicstaticboolTryEncodeToUtf8InPlace(Span<byte>buffer,intdataLength,outintbytesWritten);publicstaticOperationStatusDecodeFromUtf8(ReadOnlySpan<byte>source,Span<byte>destination,outintbytesConsumed,outintbytesWritten,boolisFinalBlock=true);publicstaticintDecodeFromUtf8(ReadOnlySpan<byte>source,Span<byte>destination);publicstaticboolTryDecodeFromUtf8(ReadOnlySpan<byte>source,Span<byte>destination,outintbytesWritten);publicstaticbyte[]DecodeFromUtf8(ReadOnlySpan<byte>source);publicstaticOperationStatusDecodeFromChars(ReadOnlySpan<char>source,Span<byte>destination,outintcharsConsumed,outintbytesWritten,boolisFinalBlock=true);publicstaticintDecodeFromChars(ReadOnlySpan<char>source,Span<byte>destination);publicstaticboolTryDecodeFromChars(ReadOnlySpan<char>source,Span<byte>destination,outintbytesWritten);publicstaticbyte[]DecodeFromChars(ReadOnlySpan<char>source);publicstaticintDecodeFromUtf8InPlace(Span<byte>buffer);publicstaticboolIsValid(ReadOnlySpan<char>base64UrlText);publicstaticboolIsValid(ReadOnlySpan<char>base64UrlText,outintdecodedLength);publicstaticboolIsValid(ReadOnlySpan<byte>utf8Base64UrlText);publicstaticboolIsValid(ReadOnlySpan<byte>utf8Base64UrlText,outintdecodedLength);}

Draft PR until below list is completed, for now mainly for checking perf, run tests on different CI legs, and get early feedback.

  • Handle '%' in decoding as a valid padding character for Base64Url
  • Doc needs improvement
  • A few APIs missing unit tests
  • Fix failures on ARM CI legs

Fixes#1658

samsosa, WeihanLi, and brentschmaltz reacted with heart emoji
@ghost
Copy link

Note regarding thenew-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link
Member

@gfoidlgfoidl left a comment

Choose a reason for hiding this comment

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

Just skimmed over it...
I saw your comment, but for checking perf some of my comments may help (a bit).

buyaa-n reacted with thumbs up emoji
buyaa-nand others added2 commitsMay 17, 2024 11:00
Co-authored-by: Günther Foidl <gue@korporal.at>
@buyaa-n
Copy link
ContributorAuthor

/benchmark

@buyaa-n
Copy link
ContributorAuthor

Just skimmed over it... I saw your comment, but for checking perf some of my comments may help (a bit).

Thank you, sure perf related feedbacks appreciated, I updated the description, any feedbacks are welcome.

@pr-benchmarksPR Benchmarks
Copy link

Crank Pull Request Bot

/benchmark <benchmark[,...]> <profile[,...]> <component,[...]> <arguments>

Benchmarks:

  • micro: .NET Performance micro benchmarks (set filter by adding--variable filter=...; by defaultfilter=*LinqBenchmarks*)
  • plaintext: TechEmpower Plaintext Scenario - ASP.NET Platform implementation
  • json: TechEmpower JSON Scenario - ASP.NET Platform implementation
  • fortunes: TechEmpower Fortunes Scenario - ASP.NET Platform implementation
  • fortunes_ef: TechEmpower Fortunes Scenario with EF Core - ASP.NET Platform implementation
  • httpclient: HttpClient Benchmark (change HTTP version by adding e.g.--variable httpVersion=3.0; change response size by adding e.g.--variable responseSize=256; default: HTTP/1.1 GET 8K)

Profiles:

  • aspnet-citrine-lin: Intel/Linux 28 Cores
  • aspnet-perf-lin: Intel/Linux 12 Cores
  • aspnet-citrine-amd: Amd/Linux 48 Cores

Components:

  • runtime
  • libs

Arguments: any additional arguments to pass through to crank, e.g.--variable name=value

@pr-benchmarksPR Benchmarks

This comment was marked as duplicate.

@pr-benchmarksPR Benchmarks

This comment was marked as duplicate.

@pr-benchmarksPR Benchmarks

This comment was marked as duplicate.

buyaa-nand others added2 commitsJune 10, 2024 21:23
Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
padding++;
}

if (TBase64Decoder.IsValidPadding(Unsafe.Subtract(ref ptrToLastElement, 1)))
Copy link
Member

Choose a reason for hiding this comment

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

What guarantees this is in bounds?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

This is only called with a buffer that has size of 4

constintBlockSize=4;
Span<byte>buffer=stackallocbyte[BlockSize];

intpaddingCount=GetPaddingCount<TBase64Decoder>(refbuffer[^1]);


if (src == srcEnd)
goto DoneExit;
}

end = srcMax -64;
end = srcMax -32;
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change from 64 to 32?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

It was 32 originally, but unintentionally changed withthis PR

The method comment also mentions it require 32 byte

privatestaticunsafevoidAvx2Encode<TBase64Encoder,T>(refbyte*srcBytes,refT*destBytes,byte*srcEnd,intsourceLength,intdestLength,byte*srcStart,T*destStart)
whereTBase64Encoder:IBase64Encoder<T>
whereT: unmanaged
{
// If we have AVX2 support, pick off 24 bytes at a time for as long as we can.
// But because we read 32 bytes at a time, ensure we have enough room to do a
// full 32-byte read without segfaulting.

Copy link
Member

@stephentoubstephentoub left a comment

Choose a reason for hiding this comment

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

Thanks!

buyaa-n reacted with thumbs up emoji
{
constint BlockSize =4;
int BlockSize =Math.Min(source.Length - (int)sourceIndex, 4);
Copy link
Member

Choose a reason for hiding this comment

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

Would anything bad happen if this were left at a const4?
This not being const would penalize the non-url path as well a bit.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

ForBase64Url when the source is not multiple of 4 we need to adjust the buffer size accordingly, theBlockSize value used to fill the whitespace with padding (row 653) so that it could decoded correctly (in case remaining bytes were decodable or valid)

@MihaZupan
Copy link
Member

@MihuBot fuzz Base64

@lewing
Copy link
Member

lewing commentedJun 19, 2024•
edited
Loading

also showing regressions indotnet/perf-autofiling-issues#36512

and improvementsdotnet/perf-autofiling-issues#36643

Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@gfoidlgfoidlgfoidl left review comments

@jkotasjkotasjkotas left review comments

@bartonjsbartonjsbartonjs left review comments

@MihaZupanMihaZupanMihaZupan left review comments

@samsosasamsosasamsosa left review comments

@stephentoubstephentoubstephentoub approved these changes

@tannergoodingtannergoodingAwaiting requested review from tannergooding

@kunalspathakkunalspathakAwaiting requested review from kunalspathak

Assignees

@buyaa-nbuyaa-n

Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Base 64 URL
8 participants
@buyaa-n@MihaZupan@lewing@stephentoub@gfoidl@jkotas@bartonjs@samsosa
[8]ページ先頭
©2009-2025 Movatter.jp