Skip to content
This repository was archived by the owner on Nov 20, 2018. It is now read-only.

Improve DefaultHttpXxx default paths #814

Closed
wants to merge 1 commit into from

Conversation

benaadams
Copy link
Contributor

@benaadams benaadams commented Apr 10, 2017

  • Reduce the number of (non-inlining) calls via interface to features get_Version
  • Set up IHttpRequestFeature and IHttpResponseFeature while creating the context

Additionally there are compromises in DefaultHttpXxx for pooling; however this can be achieved without these compromises as well as improving the call chains to support devirtualization https://github.com/dotnet/coreclr/issues/9908.

  • Implement PooledHttpContext though DefaultHttpContext rather than via inheritance.
  • seal DefaultHttpXxx types to support devirtualization of overriden methods
  • make DefaultHttpXxx private members direct types rather than parent types to support devirtualization
  • Remove and don't call virtual methods from DefaultHttpXxx .ctors
  • Make virtual methods in DefaultHttpXxx non-virtual

Non-DefaultHttpXxx can still be archived by inheriting from HttpXxx and either implementing via wrapping DefaultHttpXxx as per the Pooling example to make use of the feature caches; or by doing something alternative and using a completely different implementation.

@benaadams
Copy link
Contributor Author

ApiCheck issues

@benaadams
Copy link
Contributor Author

Related to call costs #811

@benaadams
Copy link
Contributor Author

benaadams commented Apr 10, 2017

Before

                   Method |        Mean |    StdDev |         Op/s | Allocated |
------------------------- |------------ |---------- |------------- |---------- |
            CreateContext | 143.4606 ns | 0.9601 ns |   6970556.99 |     296 B |
           DisposeContext |   4.6421 ns | 0.0318 ns | 215420903.55 |       0 B |
              GetResponse |   3.3653 ns | 0.0784 ns | 297146866.37 |       0 B |
    GetResponseBodyCached |  11.0266 ns | 0.0513 ns |  90689729.30 |       0 B |
  GetResponseBodyUncached |  79.8499 ns | 0.3087 ns |  12523500.05 |       0 B |
   GetContentLengthCached |  31.3262 ns | 0.1729 ns |  31922152.25 |       0 B |
 GetContentLengthUncached | 103.7163 ns | 0.6423 ns |   9641681.62 |       0 B |

After

                   Method |        Mean |    StdDev |         Op/s | Allocated |
------------------------- |------------ |---------- |------------- |---------- |
            CreateContext | 143.7290 ns | 1.4642 ns |   6957536.80 |     296 B |
           DisposeContext |   4.3784 ns | 0.0212 ns | 228396085.62 |       0 B |
              GetResponse |   3.5320 ns | 0.0270 ns | 283129295.33 |       0 B |
    GetResponseBodyCached |  11.6523 ns | 0.0518 ns |  85819695.93 |       0 B |
  GetResponseBodyUncached |  64.5821 ns | 0.4103 ns |  15484163.61 |       0 B |
   GetContentLengthCached |  30.4709 ns | 0.2116 ns |  32818250.53 |       0 B |
 GetContentLengthUncached |  87.3118 ns | 0.7927 ns |  11453201.08 |       0 B |

Affects the uncached calls; didn't seem to really improve general call chain otherwise 😢

May need yet to be implmented devirualization/inlines

@benaadams
Copy link
Contributor Author

Think also need to cut down on the amplification of (non-inlining) calls via interface to get_Version

1,002,263 calls for 143,180 plaintext requests

image

@benaadams
Copy link
Contributor Author

Halved number of calls to revision

image

namespace Microsoft.AspNetCore.Http.Performance
{
[Config(typeof(CoreConfig))]
public class DefaultContextBenchmark
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would commit these separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorta; will get rid of the extras

@benaadams benaadams changed the title Remove DefaultHttpContext compromises for pooling Improve DefaultHttpXxx default paths Apr 11, 2017
@benaadams
Copy link
Contributor Author

UpdateFeatures_ClearsCachedFeatures is a crazy test :-/

@muratg
Copy link

muratg commented Aug 15, 2017

@benaadams is this an experimentation or a WIP for an actual change? :)

@benaadams
Copy link
Contributor Author

Was an actual change; let me work out what it was doing again...

@benaadams
Copy link
Contributor Author

I'll close and reopen :D

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

Successfully merging this pull request may close these issues.

4 participants