-
Notifications
You must be signed in to change notification settings - Fork 413
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
In JsonWebToken use the values as bytes instead of converting to string #2535
base: dev
Are you sure you want to change the base?
Conversation
…claims as UTF8 ReadOnlyMemory<byte>.
…value indices instead of creating Memor<byte> for each claim.
{ | ||
ArrayPool<byte>.Shared.Return(output, true); | ||
} | ||
byte[] output = new byte[outputSize]; |
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.
Is it possible to still pool this buffer? Maybe by making JsonClaimSet
disposable (or some new type that wraps a JsonClaimSet
and a buffer), and it returns the buffer back to the pool once it has been disposed of.
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.
This was just simple change to make POC work. Yep, sounds like that could work.
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.
@eerhardt JsonWebToken is referenced in the ClaimsIdentity, which is not disposable currently. Even if we make ClaimsIdentity disposable, we still don't control how and for how long it's used. And adding the buffer clean up in a finalizer also doesn't clean up promptly. Any ideas on how to clean up buffer in this scenario?
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.
Yeah, having an IDisposable object hold on to an ArrayPool array leads to issues, like the one solved here: #2178. Especially if people don't expect to have to dispose of the object.
One (not necessarily great) idea I have that you could try is to still use a pooled array here, and copy out each individual byte[]
for each claim value. It might reduce allocations from this change, since you wouldn't be allocating the full buffer, but instead a bunch of smaller buffers with only the values you need.
But for this to actually be beneficial you can't get a string
for the value later. Or else you would just be allocating the values 2x: once for the byte[]
and once for the string
.
My suggestion would be to try the different approaches in a more end-to-end benchmark app to see how they behave.
And adding the buffer clean up in a finalizer also doesn't clean up promptly.
I wouldn't suggest using a finalizer. IDisposable is the right pattern to use if we want a determistic clean up of a pooled resource.
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 tried something similar in Commit 5 - copying the bytes into Memory. I can try to run a benchmark on this.
I think we could mostly use bytes instead of string throughout our code. Although there are some exceptions like in auditing code, we still need a string. So for backwards compat we still need to provide a way to get a string out of bytes.
@@ -570,16 +570,9 @@ internal JsonClaimSet CreateClaimSet(ReadOnlySpan<char> strSpan, int startIndex, | |||
{ | |||
int outputSize = Base64UrlEncoding.ValidateAndGetOutputSize(strSpan, startIndex, length); | |||
|
|||
byte[] output = ArrayPool<byte>.Shared.Rent(outputSize); |
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.
Ran some benchmarks to compare using and not using ArrayPool for buffers when reading header and payload in a token. The benchmarks use token with just a few claims, so the different could be bigger with larger tokens.
Not using ArrayPool is slightly faster (although insignificant when comparing E2E) and has more allocations and Gen0. Intersting that JwtSecurityTokenHandler_ValidateTokenAsync doesn't show allocation increase.
Use ArrayPool | Method | Mean | Error | StdDev | P90 | P95 | P100 | Gen0 | Allocated |
---|---|---|---|---|---|---|---|---|---|
Yes | ReadJWS_FromMemory | 3.061 μs | 0.0630 μs | 0.1342 μs | 3.293 μs | 3.306 μs | 3.317 μs | 0.1602 | 2.67 KB |
No | ReadJWS_FromMemory | 2.990 μs | 0.0220 μs | 0.0478 μs | 3.052 μs | 3.059 μs | 3.083 μs | 0.1793 | 2.97 KB |
Yes | ReadJWS_FromString | 3.036 μs | 0.0095 μs | 0.0205 μs | 3.066 μs | 3.071 μs | 3.073 μs | 0.1602 | 2.67 KB |
No | ReadJWS_FromString | 2.969 μs | 0.0198 μs | 0.0426 μs | 3.031 μs | 3.035 μs | 3.043 μs | 0.1793 | 2.97 KB |
Yes | JsonWebTokenHandler_ValidateTokenAsync | 25.64 μs | 0.024 μs | 0.053 μs | 25.71 μs | 25.72 μs | 25.81 μs | 0.2441 | 4.01 KB |
No | JsonWebTokenHandler_ValidateTokenAsync | 25.55 μs | 0.066 μs | 0.143 μs | 25.69 μs | 25.72 μs | 25.78 μs | 0.2441 | 4.3 KB |
Yes | JwtSecurityTokenHandler_ValidateTokenAsync | 28.46 μs | 0.051 μs | 0.111 μs | 28.60 μs | 28.65 μs | 28.76 μs | 0.7324 | 12.02 KB |
No | JwtSecurityTokenHandler_ValidateTokenAsync | 28.32 μs | 0.050 μs | 0.109 μs | 28.47 μs | 28.52 μs | 28.55 μs | 0.7324 | 12.02 KB |
test/Microsoft.IdentityModel.JsonWebTokens.Tests/JsonWebTokenTests.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.JsonWebTokens/Json/JsonWebToken.HeaderClaimSet.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.JsonWebTokens/Json/JsonWebToken.PayloadClaimSet.cs
Outdated
Show resolved
Hide resolved
… vs bytes issuer.
Comparison of .NET's SequenceEquals and our Utility.AreEqual to compare byte Spans. The test cases include short and long strings, matching and non-matching comparisons. The results show
public static bool AreEqualVariable(ReadOnlySpan<byte> a, ReadOnlySpan<byte> b)
{
if (((a.IsEmpty) || (b.IsEmpty))
|| (a.Length != b.Length))
{
return false;
}
for (int i = 0; i < a.Length; i++)
{
if ((a[i] ^ b[i]) > 0)
return false;
}
return true;
} [GroupBenchmarksBy(BenchmarkDotNet.Configs.BenchmarkLogicalGroupRule.ByCategory)]
public class SequenceEqualTests
{
private ReadOnlyMemory<byte> _issuer1;
private ReadOnlyMemory<byte> _issuer2;
private ReadOnlyMemory<byte> _issuer3;
private ReadOnlyMemory<byte> _issuer4;
private ReadOnlyMemory<byte> _token1;
private ReadOnlyMemory<byte> _token2;
[GlobalSetup]
public void Setup()
{
_issuer1 = Encoding.UTF8.GetBytes("http://Default.Issuer.com").AsMemory();
_issuer2 = Encoding.UTF8.GetBytes("http://Default.Issuer.com").AsMemory();
_issuer3 = Encoding.UTF8.GetBytes("http://Default.Issuer.com/").AsMemory();
_issuer4 = Encoding.UTF8.GetBytes("attp://Default.Issuer.com").AsMemory();
_token1 = Encoding.UTF8.GetBytes("t7UrjI0YkCuGyFWWWMHzu3jLgH0MMUP53A2D7HvCkuzswETnNwqB9rAzKfepmdlbk14ITSu4KOc9QfKjBb4FgC2Wrw8sLSxeKUKkizxoQHMpVfBXuMKtJHi8KW3sC3QZpnkanJbJuN7EahR5iGeCTX9r8cuYO3HpZ3SgJFFbCFvasYhE0VOzO0BZma5r7KO6xDaPAPzQCr2hUfGrPjHM0xSLnddpYn9wmaDm93FYXIHspgCufsEguGVtLKagWiBPDJfZOpc9JZdV9yLeL0yygbhlJUocBrp1Ux9y0KTIQ2Vz2qM4Af5ENVQnqBBvZ6HwIa8o2fS0RkbBOeSUGvlr2bl5qJXz3rEtYs2yNXgvqYEQllf9Nx8B2f2DbN4vga3muAsBI7EgeauiL241jkVu2hTXVJ1iZaadJ4O1aWoH07uYPU37rTfRZHi0pk6agVOdg8d6uLcOeuoWgnDPy6agxvG2BjnDKbTWFIr4YLFSc59zguUFH8gQjima3FqY3ASDGCFq4ti1XNZEaUwPgLHTVbDbuFGok0YfhxfoWckVcHksqWY3LuywG0w617SPKG39bMuJlb4sIq612VHXqSylByGsnS0EIBw1XzZxsfl5AQgud406QmL59K0K68VW0KvMyEbesuccgCLThAY5BFV5Zf2WIRuOnDxwPybjhQBFiTII0okshSg7o8pUOwfVJz7ALn8Ttt3LHOtQNmbE9HtvJ3tdZtcupaVxG67tMBWHoPRmG6xiyoHc1AF9vvvD4TTlSNwl3590CPcFPmsVPLLjQmPwUkbjyvluDEgkL8m7cJ1ue3kcwOq5Fjw3tysUIdx8v8t3TrmVNlXMJHDwcWiifgEvoJ2E7nSLUcGhjosc8xUQGRyTXvH1SXIWeVpy6oWEuXgmfCJmkshNdAB0XzCRbnGemXmeM4l47kKQIlQqGJ6CIIMWPPIZA6ILdPXS2Xak1I4Tj3iM42f0qkiJoBmOGugjF80i6bvru9wU3MI7KGcFfmHArnvTIdq9daS0aYBgKYHs0asg2phb8W7e5Xgb4YUpPbZ9uKHz2xewhGc45rxhaZpp58VMd3ErIV26hRNg6qwCmvVUSnOtBnW6EfG6GC221EcRE9uUizg9dlxLaur8RDUwt2Z1fYlvZISG7iu5APa8Cyekf7cwao2w57u4idamWG3pAmJNovHnxajP77P3uZ66xuGEg9sHnbYHzX08EgA3z0IQkvq5KMN57RR9OLRblHNCIndXFPTlu9pzpDrbMQPMIFZeYv23NChBC5A49r5IHbLz4zDmLWEXb2tUxRmnviLmCFSm2TQCvkbzOFhHnQxRGw1OI31tJOzoXg041XMYtUYGG6AoG84fwe05ETzdb6w8npGMciSrH9xN72I67uuuIbIOdQ3oeChLaZ2eMyQ2fcCUZJrqkxeoQNzuo1ViGgiBrkctUqMlOX4LF8hXeyrawjVYJNZztWsYExuzc1EI3Sz77eAF5ZgMvEpFVfWdCFJQ3EMlpB2HU3ofBD52N2Eo7CCLInIqGEsrQmt4HLiqC5eKs1zmRH4gjz5kcInx31yj0rX0qYIk60vTvNRj5q0Gf2KlM4v3FASI3Dx9jlhTGwYE4fSGKSLztvS43CHe4m2seUFv5ZZC1ueLdCkiKs8phDfBxz7Gy4XJIejGHAHyME02MiiUvug6MOTXr2MZRGxGKbMuWq5qDqIYI2Foz10sgvt1JdghxVKviVIzF3nbWTOs5ef44Z7xDgfb5Vqaq0qRaQBpBuFtwDgnIv9nh660yr3eX1OYxqXrNKOL2BtrUiOdM8jVM9ixqONbLYjblNx65KkCo5WYSB8ylS01o1LpQ3ZdBsu1lOvn0KdalfLMB2skdz5j8NRclKLDxq1Yzhb75fnqXsM5RmR5b0MagMf8wECybI9TF5crnq8PAYtVOseIfMEDSRhLWSMQDkJc5N8ov2oYjBmll041nfBZIFOBBd2yCTEVrq69B8RVqMHcr2lw083Ew1Q12U3o5b63wDVXTCi5ApDz9b7nt8GRs9uiQsBjXkBB3fKMxAf4jVgZRaDQ4k5NaNAzvmzGgVviqK4QLTIeubDyDXpHGwHnmqQB9Rw6mdaR1aglnsvgYR8kd40aXgsMthz47kJKYZz7KBYYD98xmneu75UA9CLd8iLPIqWoCH04Mk1xdCvjyWR4m9VJYC6lH4x3U4Qgmkt9").AsMemory();
_token2 = Encoding.UTF8.GetBytes("t7UrjI0YkCuGyFWWWMHzu3jLgH0MMUP53A2D7HvCkuzswETnNwqB9rAzKfepmdlbk14ITSu4KOc9QfKjBb4FgC2Wrw8sLSxeKUKkizxoQHMpVfBXuMKtJHi8KW3sC3QZpnkanJbJuN7EahR5iGeCTX9r8cuYO3HpZ3SgJFFbCFvasYhE0VOzO0BZma5r7KO6xDaPAPzQCr2hUfGrPjHM0xSLnddpYn9wmaDm93FYXIHspgCufsEguGVtLKagWiBPDJfZOpc9JZdV9yLeL0yygbhlJUocBrp1Ux9y0KTIQ2Vz2qM4Af5ENVQnqBBvZ6HwIa8o2fS0RkbBOeSUGvlr2bl5qJXz3rEtYs2yNXgvqYEQllf9Nx8B2f2DbN4vga3muAsBI7EgeauiL241jkVu2hTXVJ1iZaadJ4O1aWoH07uYPU37rTfRZHi0pk6agVOdg8d6uLcOeuoWgnDPy6agxvG2BjnDKbTWFIr4YLFSc59zguUFH8gQjima3FqY3ASDGCFq4ti1XNZEaUwPgLHTVbDbuFGok0YfhxfoWckVcHksqWY3LuywG0w617SPKG39bMuJlb4sIq612VHXqSylByGsnS0EIBw1XzZxsfl5AQgud406QmL59K0K68VW0KvMyEbesuccgCLThAY5BFV5Zf2WIRuOnDxwPybjhQBFiTII0okshSg7o8pUOwfVJz7ALn8Ttt3LHOtQNmbE9HtvJ3tdZtcupaVxG67tMBWHoPRmG6xiyoHc1AF9vvvD4TTlSNwl3590CPcFPmsVPLLjQmPwUkbjyvluDEgkL8m7cJ1ue3kcwOq5Fjw3tysUIdx8v8t3TrmVNlXMJHDwcWiifgEvoJ2E7nSLUcGhjosc8xUQGRyTXvH1SXIWeVpy6oWEuXgmfCJmkshNdAB0XzCRbnGemXmeM4l47kKQIlQqGJ6CIIMWPPIZA6ILdPXS2Xak1I4Tj3iM42f0qkiJoBmOGugjF80i6bvru9wU3MI7KGcFfmHArnvTIdq9daS0aYBgKYHs0asg2phb8W7e5Xgb4YUpPbZ9uKHz2xewhGc45rxhaZpp58VMd3ErIV26hRNg6qwCmvVUSnOtBnW6EfG6GC221EcRE9uUizg9dlxLaur8RDUwt2Z1fYlvZISG7iu5APa8Cyekf7cwao2w57u4idamWG3pAmJNovHnxajP77P3uZ66xuGEg9sHnbYHzX08EgA3z0IQkvq5KMN57RR9OLRblHNCIndXFPTlu9pzpDrbMQPMIFZeYv23NChBC5A49r5IHbLz4zDmLWEXb2tUxRmnviLmCFSm2TQCvkbzOFhHnQxRGw1OI31tJOzoXg041XMYtUYGG6AoG84fwe05ETzdb6w8npGMciSrH9xN72I67uuuIbIOdQ3oeChLaZ2eMyQ2fcCUZJrqkxeoQNzuo1ViGgiBrkctUqMlOX4LF8hXeyrawjVYJNZztWsYExuzc1EI3Sz77eAF5ZgMvEpFVfWdCFJQ3EMlpB2HU3ofBD52N2Eo7CCLInIqGEsrQmt4HLiqC5eKs1zmRH4gjz5kcInx31yj0rX0qYIk60vTvNRj5q0Gf2KlM4v3FASI3Dx9jlhTGwYE4fSGKSLztvS43CHe4m2seUFv5ZZC1ueLdCkiKs8phDfBxz7Gy4XJIejGHAHyME02MiiUvug6MOTXr2MZRGxGKbMuWq5qDqIYI2Foz10sgvt1JdghxVKviVIzF3nbWTOs5ef44Z7xDgfb5Vqaq0qRaQBpBuFtwDgnIv9nh660yr3eX1OYxqXrNKOL2BtrUiOdM8jVM9ixqONbLYjblNx65KkCo5WYSB8ylS01o1LpQ3ZdBsu1lOvn0KdalfLMB2skdz5j8NRclKLDxq1Yzhb75fnqXsM5RmR5b0MagMf8wECybI9TF5crnq8PAYtVOseIfMEDSRhLWSMQDkJc5N8ov2oYjBmll041nfBZIFOBBd2yCTEVrq69B8RVqMHcr2lw083Ew1Q12U3o5b63wDVXTCi5ApDz9b7nt8GRs9uiQsBjXkBB3fKMxAf4jVgZRaDQ4k5NaNAzvmzGgVviqK4QLTIeubDyDXpHGwHnmqQB9Rw6mdaR1aglnsvgYR8kd40aXgsMthz47kJKYZz7KBYYD98xmneu75UA9CLd8iLPIqWoCH04Mk1xdCvjyWR4m9VJYC6lH4x3U4Qgmkt9").AsMemory();
}
[Benchmark(Baseline = true)]
[BenchmarkCategory("ShortString_Matches")]
public bool SequenceEqual_ShortString_Matches()
{
return _issuer1.Span.SequenceEqual(_issuer2.Span);
}
[Benchmark(Baseline = true)]
[BenchmarkCategory("LongString_Matches")]
public bool SequenceEqual_LongString_Matches()
{
return _token1.Span.SequenceEqual(_token2.Span);
}
[Benchmark(Baseline = true)]
[BenchmarkCategory("DoesntMatch_DifferentLength")]
public bool SequenceEqual_DoesntMatch_DifferentLength()
{
return _issuer1.Span.SequenceEqual(_issuer3.Span);
}
[Benchmark(Baseline = true)]
[BenchmarkCategory("DoesntMatch_SameLength")]
public bool SequenceEqual_DoesntMatch_SameLength()
{
return _issuer1.Span.SequenceEqual(_issuer4.Span);
}
[Benchmark]
[BenchmarkCategory("ShortString_Matches")]
public bool UtilityAreEqual_ShortString_Matches()
{
return Utility.AreEqual(_issuer1.Span, _issuer2.Span);
}
[Benchmark]
[BenchmarkCategory("LongString_Matches")]
public bool UtilityAreEqual_LongString_Matches()
{
return Utility.AreEqual(_token1.Span, _token2.Span);
}
[Benchmark]
[BenchmarkCategory("DoesntMatch_DifferentLength")]
public bool UtilityAreEqual_DoesntMatch_DifferentLength()
{
return Utility.AreEqual(_issuer1.Span, _issuer3.Span);
}
[Benchmark]
[BenchmarkCategory("DoesntMatch_SameLength")]
public bool UtilityAreEqual_DoesntMatch_SameLength()
{
return Utility.AreEqual(_issuer1.Span, _issuer4.Span);
}
[Benchmark]
[BenchmarkCategory("ShortString_Matches")]
public bool UtilityAreEqualVariable_ShortString_Matches()
{
return Utility.AreEqualVariable(_issuer1.Span, _issuer2.Span);
}
[Benchmark]
[BenchmarkCategory("LongString_Matches")]
public bool UtilityAreEqualVariable_LongString_Matches()
{
return Utility.AreEqualVariable(_token1.Span, _token2.Span);
}
[Benchmark]
[BenchmarkCategory("DoesntMatch_DifferentLength")]
public bool UtilityAreEqualVariable_DoesntMatch_DifferentLength()
{
return Utility.AreEqualVariable(_issuer1.Span, _issuer3.Span);
}
[Benchmark]
[BenchmarkCategory("DoesntMatch_SameLength")]
public bool UtilityAreEqualVariable_DoesntMatch_SameLength()
{
return Utility.AreEqualVariable(_issuer1.Span, _issuer4.Span);
}
} |
Performance comparison: Reading (creating) JsonWebToken:
The above table shows results when
Validate token:
Commit f013da8 compares using issuer delegates comparing string issuers and byte issuers in this feature branch. The first two rows show that comparison. The third row shows comparing strings with the current, unchanged code.
|
Made changes in this commit. Brought back the ArrayPool for the whole token bytes. Saving only the bytes for string header and payload claims. Here are the results. The "Before" is dev branch, "After 1" is original changes - not using the ArrayPool and keeping all the bytes; and "After 2" is with the above commit. The allocations aren't that bad now, but still an increase. Also a small increase in latency; my guess is because we have to copy over the bytes when we use Memory.
|
Fixes #2583.
Changes:
Aside:
Commit 4 shows a second approach. Instead of saving the value indices and looking into the original token bytes; it copies the claim bytes into a separate Memory instances which are saved into a dictionary. This is not ideal as it creates duplicate memory.
Performance comparison:
Overall: #2535 (comment)
Benchmark results of comparing byte Spans: #2535 (comment)