-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
perf: improve allocations in OwinEnvironment
#58917
base: main
Are you sure you want to change the base?
perf: improve allocations in OwinEnvironment
#58917
Conversation
TODO:
|
ebd529e
to
ebc7e4d
Compare
src/Http/Owin/src/OwinEnvironment.cs
Outdated
|
||
private sealed class OwinEntries : IEnumerable<KeyValuePair<string, FeatureMap>> | ||
{ | ||
private static readonly IDictionary<string, FeatureMap> _entries = new Dictionary<string, FeatureMap> |
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.
if immutable dictionary is available, I wonder if we should use that instead, to advertise 100% "this will not change"; alternatively (other than for the deferred per-context populate, which might need thought), I wonder if this could just be a switch
expression...
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.
changed type of _entries
to IReadonlyDictionary<string, FeatureMap>
+ creating the _entries
as ImmutableDictionary
to completely make sure we are not breaking the shared-state dictionary.
src/Http/Owin/src/OwinEnvironment.cs
Outdated
} | ||
else | ||
{ | ||
foreach (var entry in _entries.Union(_contextDependentEntries)) |
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.
Union
isn't free; probably isn't an issue, but this might be something to check the impact of in pathological cases; since you have two dictionaries, it might be more efficient to hand roll this with a foreach
and ContainsKey
/ TryGetValue
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.
rewritten into 2 consequent foreach loops
concept looks solid, nice; added some thoughts |
could you please resolve conflict @DeagleGross |
@@ -119,4 +121,40 @@ bool IDictionary<string, StringValues>.TryGetValue(string key, out StringValues | |||
value = default(StringValues); | |||
return false; | |||
} | |||
|
|||
public struct Enumerator : IEnumerator<KeyValuePair<string, StringValues>>, IEnumerator |
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 this not share as public struct Enumerator : IEnumerator<KeyValuePair<string, T>>, IEnumerator
with the one above? ConvertingEnumerator or something
|
||
if (_entries.ContainsKey(key) || _contextDependentEntries.ContainsKey(key)) | ||
{ | ||
_deletedKeys ??= new HashSet<string>(); |
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.
looks like we're not consistent but new HashSet<string>(StringComparer.Ordinal)
is more readable?
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.
similar comment for all the string keyed dictionaries
|
||
foreach (var entry in _contextDependentEntries) | ||
{ | ||
if (_deletedKeys?.Contains(entry.Key) == true) |
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.
if (_deletedKeys?.Contains(entry.Key) == true) | |
if (_deletedKeys?.Contains(entry.Key)) |
?
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.
same elsewhere
OwinEnvironment allocates a
Dictionary<string, FeatureMap>
with at least 23 entries ofstring
andFeatureMap
objects per request.In most cases, Owin abstraction is used only to get the data in the specific format (i.e. access the HTTP method via
OwinConstants.RequestMethod
key), but not to remove \ add entries or rebuild the wholeFeatureMap
dictionary.Therefore I am introducing the
OwinEntries
class (not visible to users), which allocates the static readonly entries dictionary (similar to what existed before) and uses that for the key-value access (so a single allocation per app lifetime against the per-request allocation). However,OwinEnvironment
has a rich API to modify the dictionary (i.e. remove entries or clear them completely). Therefore I am doing the following to fully support existing API and dont introduce breaking changes:OwinEnvironment.FeatureMaps
returningIDictionary<string, FeatureMap>
there is no way to securely determine if the dictionary instance will be changed, and because of that we can't avoid performing the deep-copy of static_entries
(same perf loss as existed). Next interaction withOwinEnvironment
will be using_contextEntries
(request-lifetime) instead of static_entries
.OwinEnvironment.Remove(string key)
I am using a separateHashSet<string> _deletedKeys
to keep track of deleted entries per request lifetime. Even if all original entries are deleted, this is still a more lightweight flow than existed beforeOwinEnvironment.Clear()
I am falling back to_contextEntries
usage (request-lifetime)Note: there are some
FeatureMap
objects, which are dependent on theHttpContext
passed intoOwinEnvironment
, so I keep them separately in a dedicatedDictionary<string, FeatureMap>
. It's contains a single entry so far.I have added the microbenchmark (see PR), with a code that performs multiple requests using the default
HttpContext
, and used the newOwinEnvironment
implementation against the old one:Benchmark results:
(thanks to @deanward81 for the idea)
Closes #58916