[Perf] Add ContentLength to IHeaderDictionary #757
Conversation
| @@ -17,5 +17,10 @@ public interface IHeaderDictionary : IDictionary<string, StringValues> | |||
| /// <param name="key"></param> | |||
| /// <returns>The stored value, or StringValues.Empty if the key is not present.</returns> | |||
| new StringValues this[string key] { get; set; } | |||
|
|
|||
| /// <summary> | |||
| /// Strongly typed access to the Content-Type header. Implementations must keep this in sync with the string representation. | |||
| get | ||
| { | ||
| long value; | ||
| var rawValue = this[HeaderNames.ContentLength]; |
There was a problem hiding this comment.
So the value in the string is being parsed every time from the string? Don't we want to store this as a long somewhere that's being kept in sync with the value stored in the string dictionary?
There was a problem hiding this comment.
Yes, this is the same as before. Kestrel and WebListener will provide more optimized versions of IHeaderDictionary. I didn't optimize this one because it's primarily used in tests.
There was a problem hiding this comment.
Ah yes, I should have read your first comment more carefully before looking at the code.
| @@ -97,6 +98,34 @@ public HeaderDictionary(int capacity) | |||
| set { this[key] = value; } | |||
| } | |||
|
|
|||
| public long? ContentLength | |||
| { | |||
| get | |||
There was a problem hiding this comment.
Looks like duplication here and in DictionaryStringValuesWrapper. Should we keep this in a helper method like before and just call it from the getter and setter of the ContentLength properties.
There was a problem hiding this comment.
I thought about it but the implementation is actually different, their internal stores work a little differently.
|
@Tratcher looks good; will have a shot |
|
Probably should be |
|
I think |
|
Is already exposed as |
|
Hadn't considered that. I guess we should stay consistent then. I think 63 usable bits should be enough. We'll just need to keep guarding against negative values. |
|
|
|
Interesting that HeaderUtilities.FormatInt64 will create a string for a negative number even though HeaderUtilities.TryParseInt64 would throw trying to parse it. |
Needs PR in HttpAbstractions aspnet/HttpAbstractions#757
Tratcher
force-pushed
the
tratcher/contentlength
branch
from
001fc5d to
779115b
Compare
#407 Headers are stored as strings, but multiple components may need to examine the Content-Length header value on a request or response. To avoid redundant conversions allow both representations to be stored. The IHeaderDictionary implementations are responsible for keeping the representations in sync. I did not attempt any optimizations in the default implementations, they are primarily used for testing. The real gains will come from the WebListener and Kestrel implementations.
Do not merge this until the WebListener, Kestrel, and Hosting implementations are ready.
@benaadams Does this give you enough of an abstraction to implement in Kestrel? Do you want to take the first stab at it? The current xproj vs csproj schism may complicate matters.