Remove FeatureReference indirection #420
Conversation
|
Hi @benaadams, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
benaadams
changed the title
|
@lodejard please review. |
|
This is removing a few ints, doesn't reduce the number of objects allocated, and has much more manual code in several objects that reference features. I don't think it's worth the change... |
|
Will come back with some more analysis; but agreed its a lot more messy |
|
Its down by 80 bytes a request which if it were just ints (4 bytes) would be 20 ints; which its clearly not; so not entirely sure what's happening. Perhaps on 64bit the structs (IntPtr 8 bytes + Int 4 bytes) 8 byte align with a 4 byte padding; doubling the effect? Still seems high though. |
|
Ok, is a memory layout thing, exacerbated on x64, with the 4 memory layouts for DefaultHttpContext looking like the following As the elements can't be rearranged out of the struct for tighter packing and the pointers need to start on their boundry |
|
Hrmmm - yeah, that does seem like a really unfortunate bloating and a reasonable way to avoid it. Plus it's living in per-request memory pressure. Thanks for digging into it deeper! And if we want to keep up the manually-maintained programming pattern under control down the road we can use the Xxx.Generated.cs partial class trick for the cached-feature code paths. Don't need to do that now as part of this PR, but it's good to keep in mind if the extra code is considered unfortunate.
Last thought... We might want to keep the FeatureReference class around, if middleware ever have per-request contexts that query for particular features at multiple points in time. The FR might still be a good halfway point for those cases. |
|
Is there at least some way to share this bookkeeping code? This looks like it will cause maintenance problems. |
Could probably do something with extensions and |
|
Sample? |
|
Would have to be static methods, rather than extensions as there are nulls involved; and also a couple variants as some have different paths, but something like (updated PR with new code) |
benaadams
force-pushed
the
Features-perf
branch
from
d8c3f0b
to
2d49dcd
Compare
|
@Tratcher Something more like the PR now? I'm much happier with this design than the earlier one. |
benaadams
force-pushed
the
Features-perf
branch
from
f974c86
to
253eb92
Compare
benaadams
changed the title
benaadams
force-pushed
the
Features-perf
branch
2 times, most recently
from
25dfcad
to
fdb3d54
Compare
|
Better. Really what you've done is turn FeatureReference from a struct to a static helper class. One more comment inline. I think Lou still wants to save FeatureReference for use elsewhere. |
| return FeatureHelpers.GetOrCreateAndCache( | ||
| this, | ||
| _features, | ||
| () => new HttpAuthenticationFeature(), |
There was a problem hiding this comment.
Does this lamda bother you? Or are you relying on it getting optimized into a static field?
There was a problem hiding this comment.
It should be a compile time const I believe? Was being careful not to create any closures which would then cause runtime per-call creation.
There was a problem hiding this comment.
It will be optimized. If we want to be more anal, we can declare static delegates.
Pretty much, took a few iterations to get there though :) |
|
This looks like a good change and is much simpler than previous incantations 😄 |
benaadams
force-pushed
the
Features-perf
branch
from
c5d1726
to
e3b8f1a
Compare
Resolves aspnet#392 Requires aspnet/HttpAbstractions#420
benaadams
force-pushed
the
Features-perf
branch
2 times, most recently
from
8caf9c1
to
911bd5e
Compare
| @@ -36,6 +36,8 @@ public abstract class HttpContext : IDisposable | |||
|
|
|||
| public abstract ISession Session { get; set; } | |||
|
|
|||
| public abstract string TraceIdentifier { get; set; } | |||
There was a problem hiding this comment.
API change. I actually think this should be exposed but this change just went from being an optimization to an API change.
There was a problem hiding this comment.
Happy to back peddle; but want to put a PR on a PR and my git fu is not strong enough... 😭
There was a problem hiding this comment.
Revert the last commit and force push
There was a problem hiding this comment.
Reverted; have branched other changes to different repo
benaadams
force-pushed
the
Features-perf
branch
from
911bd5e
to
fdb3d54
Compare
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| namespace Microsoft.AspNet.Http.Features |
There was a problem hiding this comment.
@benaadams Bring this type back then I'll be ready to merge this PR. Also squash the commits.
Use FeatureCacheHelpers static methods rather than struct FeatureReference by default for lower allocation
benaadams
force-pushed
the
Features-perf
branch
from
e5b3a72
to
1f4ca55
Compare
|
Added FeatureReference back and squashed commits |
|
LGTM |
Remove FeatureReference indirection
Is more codeIs greatly improved in recent incarnation.Doesn't cause any extra casting; shouldn't be extra instructions executed; cuts
CreateHttpContextallocation by 17.5%From 1824 kB per 4k requests
To 1504 kB per 4k requests
So down by 320 kB per 4k requests; as similar saving to the
AsyncLocalchange in coreclr