-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Use typeof() rather than caching Type in static #18737
Conversation
@aspnet-hello benchmark |
Starting 'Default' pipelined plaintext benchmark with session ID 'e6f5186c786e4913a8386b101a867771'. This could take up to 30 minutes... |
Baseline
PR
|
Seems too big an improve? |
@aspnet-hello benchmark |
Starting 'Default' pipelined plaintext benchmark with session ID 'b260c8d0cf0b4e15a06b94925eb83335'. This could take up to 30 minutes... |
Baseline
PR
|
@@ -637,111 +609,111 @@ void IFeatureCollection.Set<TFeature>(TFeature feature) | |||
{ | |||
if (_currentIHttpRequestFeature != null) | |||
{ | |||
yield return new KeyValuePair<Type, object>(IHttpRequestFeatureType, _currentIHttpRequestFeature); | |||
yield return new KeyValuePair<Type, object>(typeof(IHttpRequestFeature), _currentIHttpRequestFeature); |
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.
The caching in statics would help a tiny bit here, but it is probably a drop in a bucket given the total cost of the enumerator.
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.
Nobody really calls this anyways.
Have you looked at the disassembly to verify that the optimization kicks in as expected? |
The optimization is not as good as I though. It is not obvious whether this change makes things better. You may want to keep the caching it statics, but use |
Should be resolved by dotnet/runtime#31686? |
Yes, I would expect it to handle that. |
Hello @halter73! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
From https://github.com/dotnet/corert/pull/7962/files#r373850627
/cc @jkotas