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

feat: Optimize request validator + make thread safe (backwards compatible) #660

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

Swimburger
Copy link
Contributor

@Swimburger Swimburger commented Dec 29, 2022

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

ASP.NET Core is pushing performance with every release, and I received feedback that the RequestValidator is inefficient.
This is not my area of expertise, so there's probably more optimization possible, especially if we can drop older versions of .NET, but I gave it a stab while trying to keep it backwards compatible.

The only breaking change would be that I'm doing some additional input validation which previously would have thrown null reference exceptions instead of argument exceptions. Technically a breaking change, but in reality I don't expect anyone to run into this issue.

Fixes

I created a benchmark project with a copy of the original validation code while improving the existing code.
The benchmark tests for happy path, where the parameters are passed in as a dictionary and the signature matches for the original URL, not needing to perform the validation for the second URL variant.
The unhappy path passes in a name value collection, and doesn't pass on the URL that's passed in, but does match on the variant URL.

The code is more performant because some of the string manipulations that used to be done for every URL is only done once now, and if the first signature matches, the validator doesn't bother checking for the variant URL.
Other than that, there are minor enhancements trying to reduce the amount of objects needed to perform the validation.

Here's the benchmark results for .NET 6:

BenchmarkDotNet=v0.13.3, OS=Windows 11 (10.0.22621.963)
Intel Core i7-8700 CPU 3.20GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK=7.0.101
  [Host]     : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2
  DefaultJob : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2

Method Mean Error StdDev Gen0 Gen1 Allocated
OriginalUnhappyPath 15.590 us 0.1598 us 0.1416 us 1.9836 0.2441 12.27 KB
CurrentUnhappyPath 12.526 us 0.2482 us 0.2759 us 1.3885 - 8.52 KB
OriginalHappyPath 8.906 us 0.1709 us 0.2161 us 1.6327 0.2289 10.02 KB
CurrentHappyPath 2.721 us 0.0240 us 0.0224 us 0.6256 - 3.85 KB

All the tests continue to pass.

This PR also contains the commits from PR #598 to fix the thread safety issue (fixing #466), and as a result it also improves performance. Thank you @benmccallum.

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

If you have questions, please file a support ticket, or create a GitHub Issue in this repository.

@Swimburger
Copy link
Contributor Author

Swimburger commented Dec 29, 2022

I'm leaving this PR as a draft for now, as you may want me to remove the benchmark project before pulling it in.
If you have any suggestions on how to improve the performance, let me know!

@Swimburger
Copy link
Contributor Author

Swimburger commented Dec 30, 2022

Here's an allocation report from VS when running the current unhappy path:
Report20221229-2230.zip

And the relevant call tree:

Function Name Total (Allocations) Self (Allocations) Self Size (Bytes) Module Name
 + RequestValidationBenchmark.CurrentUnhappyPath() 270 39 5,666 twilio.benchmark
     + Twilio.Security.RequestValidator.Validate(string, System.Collections.Generic.IDictionary<string, string>, string) 99 0 0 twilio.benchmark
         + Twilio.Security.RequestValidator.GetUriVariation(string) 48 3 184 twilio.benchmark
             - [External Call] System.Uri.ctor(string) 37 37 2,684 system.private.uri.il
             - [External Call] System.UriBuilder.SetFieldsFromUri() 7 7 384 system.private.uri.il
             + [Allocations] 3 184
                 -  System.UriBuilder 1 88
                 -  System.Uri 1 56
                 -  System.String 1 40
             - [External Call] System.Number.UInt32ToDecStr(uint) 1 1 30 System.Private.CoreLib.il
         + Twilio.Security.RequestValidator.GetJoinedParametersStringBuilder(System.Collections.Generic.IDictionary<string, string>) 21 2 104 twilio.benchmark
             - [External Call] System.Text.StringBuilder.AppendWithExpansion(ref char, int) 10 10 1,832 System.Private.CoreLib.il
             - [External Call] System.Array.Sort<T>(!!0[], int, int, System.Collections.Generic.IComparer<T>) 7 7 272 System.Private.CoreLib.il
             - [External Call] System.Collections.Generic.EnumerableHelpers.ToArray<T>(System.Collections.Generic.IEnumerable<T>) 1 1 184 system.linq.il
             + [Allocations] 2 104
                 -  System.Char[16] 1 56
                 -  System.Text.StringBuilder 1 48
             - [External Call] System.Collections.Generic.Dictionary<T, T>.get_Keys() 1 1 24 System.Private.CoreLib.il
         - [External Call] System.Text.StringBuilder.ToString() 3 3 1,876 System.Private.CoreLib.il
         + Twilio.Security.RequestValidator.GetValidationSignature(string) 8 0 0 twilio.benchmark
             - [External Call] System.Text.Encoding.GetBytes(string) 2 2 911 System.Private.CoreLib.il
             - [External Call] System.Security.Cryptography.HashAlgorithm.CaptureHashCodeAndReinitialize() 5 5 218 system.security.cryptography.il
             - [External Call] System.Security.Cryptography.HashAlgorithm.ComputeHash(uint8[]) 1 1 42 system.security.cryptography.il
         + Twilio.Security.RequestValidator.SetPort(string, System.UriBuilder, int) 13 5 272 twilio.benchmark
             + [Allocations] 5 272
                 -  System.String 4 224
                 -  System.Text.StringBuilder 1 48
             - [External Call] System.UriBuilder.ToString() 5 5 222 system.private.uri.il
             - [External Call] System.Text.StringBuilder.ctor(string, int, int, int) 1 1 108 System.Private.CoreLib.il
             - [External Call] System.String.Substring(int, int) 2 2 78 System.Private.CoreLib.il
         - [External Call] System.Text.StringBuilder.Insert(int, string) 4 4 322 System.Private.CoreLib.il
         - [External Call] System.Convert.ToBase64String(uint8[]) 2 2 156 System.Private.CoreLib.il
     - RequestValidationBenchmark.cctor() 83 2 160 twilio.benchmark
     - [Allocations] 39 5,666
     + Twilio.Security.RequestValidator.ToDictionary(System.Collections.Specialized.NameValueCollection) 10 1 80 twilio.benchmark
         - [External Call] System.Collections.Generic.Dictionary<T, T>.TryInsert(T, T, System.Collections.Generic.InsertionBehavior) 8 8 1,984 System.Private.CoreLib.il
         - [External Call] System.Collections.Specialized.NameValueCollection.get_AllKeys() 1 1 184 system.collections.specialized.il
         + [Allocations] 1 80
             +  System.Collections.Generic.Dictionary<System.String, System.String> 1 80
     - [External Call] System.Security.Cryptography.HMACSHA1.ctor(uint8[]) 34 34 1,542 system.security.cryptography.il
     - [External Call] System.Security.Cryptography.SHA256+Implementation.ctor() 4 4 176 system.security.cryptography.il
     - [External Call] System.Text.UTF8Encoding+UTF8EncodingSealed.GetBytesForSmallInput(string) 1 1 29 System.Private.CoreLib.il

Allocations tab:

Type Allocations
 - System.Char[] 85
 - System.String 2,248
 - System.Diagnostics.Tracing.EventSource.EventMetadata[] 7
 - System.Byte[] 342
 - System.Reflection.RuntimeParameterInfo 676
 - System.Object[] 518
 - System.Collections.Generic.Dictionary<,>.Entry[] 39
 - System.Reflection.RuntimeMethodInfo 255
 - System.Diagnostics.Tracing.EventParameterInfo[] 148
 - System.Signature 237
 - System.Int32[] 51
 - System.Reflection.ParameterInfo[] 295
 - System.RuntimeMethodInfoStub 154
 - System.UInt64[] 3
 - System.RuntimeType[] 195
 - System.Diagnostics.Tracing.EventAttribute 149
 - System.Reflection.RuntimeMethodBody 148
 - System.Reflection.CustomAttributeRecord[] 156
 - System.Reflection.RuntimeMethodInfo[] 29
 - System.Reflection.Emit.OpCode 226
 - System.Diagnostics.Tracing.EventAttribute[] 149
 - System.String[] 35
 - System.Text.StringBuilder 85
 - System.Reflection.MdFieldInfo 57
 - System.Byte 148
 - System.Diagnostics.Tracing.EventLevel 148
 - System.Reflection.RuntimeExceptionHandlingClause[0] 148
 - System.Reflection.RuntimeLocalVariableInfo[0] 148
 - System.Diagnostics.Tracing.EventKeywords 147
 - System.RuntimeType.RuntimeTypeCache 23
 - System.RuntimeType 81
 - System.Reflection.MethodInfo[] 2
 - System.Collections.Hashtable.Bucket[] 6
 - System.Reflection.RuntimeFieldInfo[] 17
 - System.Globalization.CultureData 4
 - System.Collections.Generic.Dictionary<,> 15
 - System.Reflection.Emit.DynamicILGenerator 6
 - System.Int64 37
 - System.Reflection.RuntimePropertyInfo 8
 - System.Collections.ArrayList 21
 - System.RuntimeType.RuntimeTypeCache.MemberInfoCache<> 12
 - System.Reflection.Emit.DynamicMethod 6
 - System.Reflection.FieldInfo[] 5
 - System.Collections.Specialized.NameObjectCollectionBase.NameObjectEntry 20
 - System.Diagnostics.Tracing.ScalarTypeInfo 10
 - System.Func<System.Object, System.Diagnostics.Tracing.PropertyValue> 10
 - System.Reflection.Emit.SignatureHelper 12
 - System.UriParser.BuiltInUriParser 17
 - System.Type[] 11
 - System.Reflection.RuntimePropertyInfo[] 11
 - System.Collections.Generic.List<> 14
 - System.Globalization.CultureInfo 4
 - System.Reflection.Emit.DynamicResolver 6
 - System.Reflection.RuntimePropertyInfo[][] 4
 - System.Diagnostics.Tracing.EventSource.OverrideEventProvider 4
 - System.Reflection.MethodInvoker 8
 - System.Reflection.InvokerEmitUtil.InvokeFunc 6
 - System.Diagnostics.Tracing.RuntimeEventSource 1
 - System.Object 13
 - System.Int32 12
 - System.RuntimeMethodHandle 12
 - System.Reflection.Emit.DynamicMethod.RTDynamicMethod 6
 - System.Reflection.Emit.ScopeTree 6
 - System.Reflection.CerHashtable<,>.Table 7
 - System.Collections.Concurrent.ConcurrentDictionary<System.ValueTuple<System.String, BCryptOpenAlgorithmProviderFlags>, System.ValueTuple<Microsoft.Win32.SafeHandles.SafeBCryptAlgorithmHandle, System.Int32>>.Node[31] 1
 - Interop.Advapi32.EtwEnableCallback 4
 - System.Diagnostics.Tracing.EventOpcode 9
 - System.Diagnostics.Tracing.EventTask 9
 - System.Collections.Hashtable 3
 - System.IntPtr[10] 2
 - System.Double[23] 1
 - System.Globalization.CalendarData[23] 1
 - System.Collections.Generic.HashSet<System.RuntimeType>.Entry[11] 1
 - System.UInt32 8
 - System.Reflection.Emit.GenericFieldInfo 6
 - System.Reflection.MemberFilter 3
 - System.RuntimeFieldInfoStub 3
 - System.Diagnostics.Tracing.NativeRuntimeEventSource 1
 - System.Diagnostics.Tracing.EventCommandEventArgs 2
 - System.Globalization.CalendarData 1
 - System.Diagnostics.Tracing.ManifestBuilder 1
 - System.Reflection.Emit.DynamicScope 6
 - System.RuntimeTypeHandle 6
 - System.Reflection.RtFieldInfo 2
 - Internal.Win32.SafeHandles.SafeRegistryHandle 4
 - System.Collections.Concurrent.ConcurrentDictionary<System.ValueTuple<System.String, BCryptOpenAlgorithmProviderFlags>System.ValueTuple<Microsoft.Win32.SafeHandles.SafeBCryptAlgorithmHandle, System.Int32>>.Node 2
 - System.ExecutionEngineException 1
 - System.OutOfMemoryException 1
 - System.StackOverflowException 1
 - System.Collections.Generic.Dictionary<,>.KeyCollection 5
 - System.Globalization.CompareInfo 2
 - System.Collections.Generic.KeyValuePair<SessionInfo, System.Boolean>[] 2
 - System.Reflection.RuntimeAssembly 2
 - System.Security.Cryptography.HashProviderCng 2
 - System.Diagnostics.Tracing.EventProvider.SessionInfo[8] 1
 - System.UriBuilder 1
 - System.Diagnostics.Tracing.EventSource[] 2
 - System.Collections.Specialized.NameValueCollection 1
 - System.Reflection.RuntimeFieldInfo[7][] 1
 - System.Reflection.RuntimeMethodInfo[7][] 1
 - System.RuntimeType[7][] 1
 - System.Uri.UriInfo 1
 - System.Collections.Generic.GenericArraySortHelper<> 3
 - System.Security.Cryptography.HMACSHA1 1
 - Microsoft.Win32.SafeHandles.SafeBCryptAlgorithmHandle 2
 - Microsoft.Win32.SafeHandles.SafeBCryptHashHandle 2
 - System.Collections.Generic.HashSet<System.RuntimeType> 1
 - System.Comparison<System.String> 1
 - System.Diagnostics.Tracing.EventProvider.SessionInfoCallback 1
 - System.Globalization.TextInfo 1
 - System.Reflection.RuntimeModule 1
 - System.Uri 1
 - System.Uri.MoreInfo 1
 - Internal.Win32.RegistryKey 2
 - System.Collections.Generic.GenericEqualityComparer<> 2
 - System.Collections.Generic.NonRandomizedStringEqualityComparer.OrdinalComparer 2
 - System.Diagnostics.Tracing.EtwEventProvider 2
 - System.Diagnostics.Tracing.EventPipeEventProvider 2
 - System.Diagnostics.Tracing.TraceLoggingEventHandleTable 2
 - System.WeakReference<System.Diagnostics.Tracing.EventSource> 2
 - System.Collections.Concurrent.ConcurrentDictionary<System.ValueTuple<System.String, BCryptOpenAlgorithmProviderFlags>System.ValueTuple<Microsoft.Win32.SafeHandles.SafeBCryptAlgorithmHandle, System.Int32>> 1
 - System.Collections.Concurrent.ConcurrentDictionary<System.ValueTuple<System.String, BCryptOpenAlgorithmProviderFlags>System.ValueTuple<Microsoft.Win32.SafeHandles.SafeBCryptAlgorithmHandle, System.Int32>>.Tables 1
 - System.RuntimeType.ActivatorCache 1
 - System.Security.Cryptography.HMACCommon 1
 - System.Security.Cryptography.SHA256.Implementation 1
 - System.Text.UTF8Encoding.UTF8EncodingSealed 1
 - System.Diagnostics.Tracing.EventSourceAttribute 1
 - System.WeakReference<System.Diagnostics.Tracing.EventSource>[2] 1
 - System.CultureAwareComparer 1
 - System.Diagnostics.Tracing.ActivityTracker 1
 - System.Diagnostics.Tracing.EventSourceAttribute[1] 1
 - System.Guid 1
 - Twilio.Security.RequestValidator 1
 - System.Globalization.CalendarId[1] 1
 - RequestValidationBenchmark 1
 - System.Collections.Generic.Dictionary<System.String, System.Type>.ValueCollection 1
 - System.Collections.Generic.GenericComparer<System.String> 1
 - System.Collections.Generic.NonRandomizedStringEqualityComparer.OrdinalIgnoreCaseComparer 1
 - System.Collections.Generic.ObjectEqualityComparer<System.RuntimeType> 1
 - System.DBNull 1
 - System.Diagnostics.Tracing.EventPipeMetadataGenerator 1
 - System.Diagnostics.Tracing.PropertyValue.<>c 1
 - System.OrdinalCaseSensitiveComparer 1
 - System.OrdinalIgnoreCaseComparer 1
 - System.Reflection.Missing 1
 - System.Text.DecoderReplacementFallback 1
 - System.Text.EncoderReplacementFallback 1
 - System.Type.<>c 1

We could further improve performance if we don't have to preserve the original casing of the URL, and if we didn't have to check for the URL with and without port.

@Swimburger
Copy link
Contributor Author

Pulling the code from #598 with minor modifications improves the performance and reduces allocations further.

Method Mean Error StdDev Gen0 Gen1 Allocated
OriginalUnhappyPath 15.204 us 0.1571 us 0.1226 us 1.9836 0.2441 12.27 KB
CurrentUnhappyPath 11.855 us 0.2103 us 0.3148 us 1.4191 - 8.75 KB
OriginalHappyPath 9.241 us 0.1789 us 0.1757 us 1.6327 0.2289 10.02 KB
CurrentHappyPath 3.135 us 0.0627 us 0.0586 us 0.6676 0.0038 4.09 KB

TBD if I'm merging that PR into this one.

src/Twilio/Security/RequestValidator.cs Outdated Show resolved Hide resolved
src/Twilio/Security/RequestValidator.cs Outdated Show resolved Hide resolved
@Swimburger
Copy link
Contributor Author

@davidfowl @vcsjones @benmccallum any last suggestions before I turn this into a real PR?

@benmccallum
Copy link

Much better, thanks again for pushing this along.

I've only reviewed on my phone, but it looks good to me and most importantly is thread-safe, so the docs as they were (and I imagine still are) won't lead someone down a path of great misfortune :)

@Swimburger Swimburger marked this pull request as ready for review January 17, 2023 17:55
@Swimburger Swimburger changed the title feat!: Optimize request validator but keep backwards compatible feat!: Optimize request validator + make thread safe (backwards compatible) Jan 31, 2023
@Swimburger Swimburger changed the title feat!: Optimize request validator + make thread safe (backwards compatible) feat: Optimize request validator + make thread safe (backwards compatible) Feb 3, 2023
@Swimburger
Copy link
Contributor Author

C'mon Twilio folks, this code contribution has been ready to merge on the internal repo and public repo for a long time. The longer you let these things rot, the more conflicts will arise.

By not merging this, you're choosing to keep a slower and less memory-efficient version around for no reason. 🔥 🌳

@tiwarishubham635
Copy link
Contributor

tiwarishubham635 commented Jan 18, 2024

Hello @Swimburger! I am looking at this PR from twilio. Can you please update this PR with the latest changes to ensure that the .csproj file has no conflicts? Thanks!

@Swimburger
Copy link
Contributor Author

Conflict resolved

@tiwarishubham635
Copy link
Contributor

Review in process

@benmccallum
Copy link

We even had a review from David Fowler 😄 LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants