-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
TryAddWithoutValidation for multiple values could be more efficient #102845
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.
Thanks.
@dotnet/ncl could I get a secondary pair of eyes on this one given it's based on my gist from the original issue https://gist.github.com/MihaZupan/800df7a324a66a8e814c3ec91f3d39b0
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs
Outdated
Show resolved
Hide resolved
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.
LGTM if CI is green (modulo existing comments from MihaZupan). Maybe just add a few comments for those who are not that familiar with the codebase.
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs
Outdated
Show resolved
Hide resolved
New results after adjusting performance test and TryAddWithoutValidation method :
|
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 imagine that the most common scenario for this API is a string[]
with 1 value for a header that doesn't yet exist in the collection.
In that case, we can save an allocation from avoiding the boxing of the IEnumerator.
I'm not as convinced optimizing cases like an enumerable that isn't an IList
is worth the extra maintainability cost in practice.
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs
Outdated
Show resolved
Hide resolved
The new result after the last changes (it seems the slow path underperformed by rewriting it as foreach loop that calls unitary TryAddWithoutValidation) :
|
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs
Outdated
Show resolved
Hide resolved
New benchmark results :
|
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
Fixes #64049
perfomance test dotnet/performance#4240