-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Antiforgery perf improvements (includes new DataProtection API usage) #64751
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
base: main
Are you sure you want to change the base?
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.
Pull request overview
This PR introduces significant performance improvements to the Antiforgery system by leveraging new span-based DataProtection APIs introduced in #44758. The changes eliminate buffer allocations and stream-based serialization in favor of direct span operations, resulting in substantial improvements across all measured scenarios: 13-15% reduction in allocations and 15-20% improvement in throughput.
Key Changes:
- Span-based DataProtection: Replaces byte array allocations with
ISpanDataProtectorfor zero-copy data protection operations - 7-bit encoding utilities: Adds shared utilities for efficient length-prefixed string serialization using span-based APIs
- Removed object pooling: Eliminates
AntiforgerySerializationContextpooling infrastructure now that allocations are minimal
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/Shared/Encoding/Int7BitEncodingUtils.cs |
Adds span-based methods for reading/writing 7-bit encoded integers and strings |
src/Shared/test/Shared.Tests/Encoding/Int7BitEncodingUtilsTests.cs |
Comprehensive test coverage for new encoding utilities |
src/Shared/WebEncoders/WebEncoders.cs |
Adds Base64Url decoding directly to span for .NET 9+ |
src/Antiforgery/src/Internal/DefaultClaimUidExtractor.cs |
Converts to span-based claim UID extraction using stack allocation and direct SHA256 hashing |
src/Antiforgery/src/Internal/DefaultAntiforgeryTokenSerializer.cs |
Converts to span-based serialization using ISpanDataProtector API |
src/Antiforgery/src/Internal/DefaultAntiforgeryTokenGenerator.cs |
Updates to use span-based claim UID comparison |
src/Antiforgery/src/Internal/AntiforgerySerializationContext.cs |
Removed - no longer needed with span-based approach |
src/Antiforgery/src/Internal/AntiforgerySerializationContextPooledObjectPolicy.cs |
Removed - pooling infrastructure no longer needed |
src/Antiforgery/src/AntiforgeryServiceCollectionExtensions.cs |
Removes object pool registration |
src/Antiforgery/test/DefaultClaimUidExtractorTest.cs |
Updates tests for new span-based API signature |
src/Antiforgery/test/DefaultAntiforgeryTokenSerializerTest.cs |
Adds TestSpanDataProtector mock implementation |
src/Antiforgery/test/DefaultAntiforgeryTokenGeneratorTest.cs |
Adds DummyClaimUidExtractor helper for testing |
src/Antiforgery/benchmarks/Microsoft.AspNetCore.Antiforgery.Benchmarks/* |
New benchmark project with comprehensive performance tests |
src/Antiforgery/src/Internal/DefaultAntiforgeryTokenSerializer.cs
Outdated
Show resolved
Hide resolved
src/Antiforgery/src/Internal/DefaultAntiforgeryTokenSerializer.cs
Outdated
Show resolved
Hide resolved
...y/benchmarks/Microsoft.AspNetCore.Antiforgery.Benchmarks/Benchmarks/AntiforgeryBenchmarks.cs
Show resolved
Hide resolved
...y/benchmarks/Microsoft.AspNetCore.Antiforgery.Benchmarks/Benchmarks/AntiforgeryBenchmarks.cs
Outdated
Show resolved
Hide resolved
src/Antiforgery/src/Internal/DefaultAntiforgeryTokenSerializer.cs
Outdated
Show resolved
Hide resolved
src/Antiforgery/src/Internal/DefaultAntiforgeryTokenGenerator.cs
Outdated
Show resolved
Hide resolved
src/Antiforgery/src/Internal/DefaultAntiforgeryTokenGenerator.cs
Outdated
Show resolved
Hide resolved
src/Antiforgery/src/Internal/DefaultAntiforgeryTokenGenerator.cs
Outdated
Show resolved
Hide resolved
src/Antiforgery/src/Internal/DefaultAntiforgeryTokenSerializer.cs
Outdated
Show resolved
Hide resolved
|
|
||
| if (_perfCryptoSystem is not null) | ||
| { | ||
| var protectBuffer = new RefPooledArrayBufferWriter<byte>(stackalloc byte[255]); |
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.
| var protectBuffer = new RefPooledArrayBufferWriter<byte>(stackalloc byte[255]); | |
| using var protectBuffer = new RefPooledArrayBufferWriter<byte>(stackalloc byte[256]); |
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.
Can remove try as well
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.
src/Antiforgery/src/Internal/DefaultAntiforgeryTokenSerializer.cs
Outdated
Show resolved
Hide resolved
src/Antiforgery/src/Internal/DefaultAntiforgeryTokenSerializer.cs
Outdated
Show resolved
Hide resolved
| if (token != null) | ||
| var tokenDecodedSize = Base64Url.GetMaxDecodedLength(serializedToken.Length); | ||
|
|
||
| var rent = tokenDecodedSize < 256 |
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.
| var rent = tokenDecodedSize < 256 | |
| var rent = tokenDecodedSize <= 256 |
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text; |
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.
| using System.Text; | |
| using System; | |
| using System.Text; |
| <Project Path="src/Antiforgery/test/Microsoft.AspNetCore.Antiforgery.Test.csproj" /> | ||
| </Folder> | ||
| <Folder Name="/src/Antiforgery/benchmarks/"> | ||
| <Project Path="src/Antiforgery/benchmarks/Microsoft.AspNetCore.Antiforgery.Benchmarks/Microsoft.AspNetCore.Antiforgery.Benchmarks.csproj" Id="1dd9d6d8-44f5-4cc9-886d-ebfec9ec289e" /> |
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.
| <Project Path="src/Antiforgery/benchmarks/Microsoft.AspNetCore.Antiforgery.Benchmarks/Microsoft.AspNetCore.Antiforgery.Benchmarks.csproj" Id="1dd9d6d8-44f5-4cc9-886d-ebfec9ec289e" /> | |
| <Project Path="src/Antiforgery/benchmarks/Microsoft.AspNetCore.Antiforgery.Benchmarks/Microsoft.AspNetCore.Antiforgery.Benchmarks.csproj" /> |
| @@ -0,0 +1,27 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
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.
Could you rename this to have .MicrosoftBenchmarks at the end
aspnetcore/Directory.Build.props
Line 30 in 5b06d1d
| <IsMicrobenchmarksProject Condition=" $(MSBuildProjectName.EndsWith('.Microbenchmarks')) ">true</IsMicrobenchmarksProject> |
| var requestTokenClaimUidLength = requestToken.ClaimUid?.Length; | ||
| if (requestTokenClaimUidLength is null) |
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.
| var requestTokenClaimUidLength = requestToken.ClaimUid?.Length; | |
| if (requestTokenClaimUidLength is null) | |
| if (requestToken.ClaimUid is null) |
| return new string(chars, startIndex: 0, length: outputLength); | ||
| if (_perfCryptoSystem is not null) | ||
| { | ||
| var protectBuffer = new RefPooledArrayBufferWriter<byte>(stackalloc byte[255]); |
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.
| var protectBuffer = new RefPooledArrayBufferWriter<byte>(stackalloc byte[255]); | |
| var protectBuffer = new RefPooledArrayBufferWriter<byte>(stackalloc byte[256]); |
| var rent = totalSize < 256 | ||
| ? stackalloc byte[255] |
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.
| var rent = totalSize < 256 | |
| ? stackalloc byte[255] | |
| var rent = totalSize <= 256 | |
| ? stackalloc byte[256] |
| private static AntiforgeryToken? Deserialize(BinaryReader reader) | ||
| private static AntiforgeryToken? Deserialize(ReadOnlySpan<byte> tokenBytes) | ||
| { | ||
| var offset = 0; |
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.
I meant more like:
var embeddedVersion = tokenBytes[0];
tokenBytes = tokenBytes.Slice(1);
if (embeddedVersion != TokenVersion)
{
return null;
}
var deserializedToken = new AntiforgeryToken();
// Read SecurityToken (16 bytes)
deserializedToken.SecurityToken = new BinaryBlob(
AntiforgeryToken.SecurityTokenBitLength,
tokenBytes[..AntiforgeryToken.SecurityTokenByteLength].ToArray());
tokenBytes = tokenBytes.Slice(AntiforgeryToken.SecurityTokenByteLength);
// Read IsCookieToken (1 byte)
deserializedToken.IsCookieToken = tokenBytes[0] != 0;
tokenBytes = tokenBytes.Slice(1);| : (tokenBytesRent = ArrayPool<byte>.Shared.Rent(tokenDecodedSize)); | ||
| var tokenBytes = rent[..tokenDecodedSize]; | ||
|
|
||
| var status = Base64Url.DecodeFromChars(serializedToken, tokenBytes, out int charsConsumed, out int bytesWritten); |
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.
I think this is a behavior change:
https://source.dot.net/#Microsoft.AspNetCore.WebUtilities/src/Shared/WebEncoders/WebEncoders.cs,133
| else | ||
| { | ||
| return token; | ||
| var unprotectedBytes = _defaultCryptoSystem.Unprotect(tokenBytesDecoded.ToArray()); |
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.
Do we still have good test coverage for the less optimized paths?

#44758 introduced new DataProtection API, and in this PR I am changing Antiforgery implementation to use a high-performance DataProtection API. Moreover, going through the implementation, I've refactored to improve performance here and there. The results are listed below.
The main idea of the improvement is to use the DataProtection API, which does not make us allocate any buffer. This is a hot-path in Antiforgery, because token serialization and deserialization may happen multiple times for different tokens per single request.
Before, we had to allocate a buffer, fill it in with the token data, pass it into the
dataProtectorand later we were using streams API, which also allocate and may be doing extra job instead of writing to the destination buffer. There are less of extra types used today, which should give some benefit to the usage.I have tested the changes via crank, but unfortunately I dont see a heavy improvement in RPS speed (around 4% RPS improvement only). I suspect those improvements mostly focus on allocation reduction, meaning in the long run it will give more RPS.
IAntiforgeryTokenSerializer
IAntiforgeryTokenGenerator
IAntiforgery
Relates to #50065