feat: remove interface for IResolveRequestContext #1176
Conversation
|
@alistairjevans as discussed in #1174, this brings back Mind running benchmarks again? |
alsami
force-pushed
the
feat/bring_back_base_resolverequestcontext
branch
from
d232291 to
3677371
Compare
Codecov Report
@@ Coverage Diff @@
## v6 #1176 +/- ##
==========================================
+ Coverage 80.18% 80.23% +0.05%
==========================================
Files 187 187
Lines 4370 4371 +1
Branches 946 947 +1
==========================================
+ Hits 3504 3507 +3
+ Misses 415 414 -1
+ Partials 451 450 -1
Continue to review full report at Codecov.
|
This is what I got on Linux, a tiny bit better. Can't test develop though. |
|
I got the same thing basically:
The red flag for me though is the fact that the original refactoring seems to have jumped the memory allocations loads... |
|
It's still 0.3 us slower than v6 before refactoring as well. Maybe it's also the type casting for the interface? |
|
Ok, just trying to narrow this down some more:
So, #1174 started consuming a lot more memory. Curious. |
|
I think I might have found the memory issue; you changed the ConcurrencyMiddleware to use .Any:
That's going to allocate an Action instance every time we hit the loop, plus the overhead of the Any LINQ construct itself. As a rule I'm trying to strip out any LINQ from the hot-path areas because it adds overhead. |
|
Makes sense, give me a second, changing it back to not use LINQ. |
|
Back to original memory-usage.
truly didn't know that a |
|
Yeah, when you pass a lambda to a method, and that lambda captures a local variable, it allocates a new instance of an object to hold that capture+delegate. It's sneaky, but shows up under a decompiler. I'll run those diff-ing benchmarks again tomorrow, then we can see what else (if anything) might need changing to bring that perf back up. |
Sounds good! |
|
@alistairjevans sorry for cluttering the PR, please feel free to delete the comments. If you want to run benchmarks, you can do so now automated. Takes a while though, even though my machine is rather powerful. |
|
Ran the benchmarks again (manually):
So, the memory issue has been fixed, and we're back to the fact that #1173 introduced the main slow down. #1173 change the resolve context to an interface, but we've just changed it back...very strange. I'll take another look at #1173. |
|
The 1173 to 1174 drop in perf were due to the additional memory allocation; the changes in this PR bring that perf back down to post-1173 levels. We've now taken One thing I've noticed is that you unsealed |
Will look into it tonight, since there is the class |
|
Actually, I've also realised that all of the properties in That virtual property access may be what's killing us. It may come down to changing the ResolveRequestContextBase back to taking constructor parameters. |
Ah, jeez. Will give it a shot later and run the benchmarks again locally. Let's see if that's the cause. If so, we need something that's more testable. Basically that would mean something like this, if we were to make it more testable this time. |
|
I admit I'm only sorta following all this. With the other reviews and such going on, I haven't had the ability to dive in and think I'd just be a distraction. The ultimate goal of having testable extensions stands. On that note, I'm holding on the DOT graph thing until this settles out. It's not a rush; I'd rather get it right than get it fast. But I don't see the value in updating it on each of these changes until we're happy with them. |
You mean you would make all of the changes requested in #1164 with a single PR? |
|
Pretty much, yeah. |
|
Any 'Testable' classes would go into the test projects though. Or we just use Moq, which has recently been added to the project. |
alsami
force-pushed
the
feat/bring_back_base_resolverequestcontext
branch
from
57fb091 to
c3e40dc
Compare
|
@alistairjevans I've applied the changes as we have agreed on. Very interesting is that the test |
|
@autofac-bot benchmark DeepGraphResolveBenchmark |
DeepGraphResolveBenchmarkTarget: v6
Complete benchmark outputSource: feat/bring_back_base_resolverequestcontext
Complete benchmark output |
|
Benchmarks looking good. |
|
@autofac-bot benchmark DeepGraphResolveBenchmark commit:944d4cb4b6b630f70229daf9de7726d5a3dbe848 |
DeepGraphResolveBenchmarkTarget: 944d4cbUnhandled exception. System.ArgumentOutOfRangeException: Length cannot be less than zero. (Parameter 'length') Complete benchmark outputSource: feat/bring_back_base_resolverequestcontext
Complete benchmark output |
alsami
changed the title
alsami
force-pushed
the
feat/bring_back_base_resolverequestcontext
branch
from
c3e40dc to
82b643e
Compare
|
Test run number3 magically passed 🤷♂️ |
|
Great, I love intermittent errors that only happen in CI. /s Looks like somethings not right with the benchmarks, look like an exception is getting output? Is there a way to indicate to the bot that we only want the results rather than the full output? |
Yeah, it's because I am publishing the benchmarks before executing them. That only works when changes from #1181 are applied. In that specific commit, that's not the case. Meaning, that for those commits, we will still have to run those benchmarks manually.
In this case you mean that you don't want to show the exception, but only the valid results, if they are present? If so, I could change the code to hide the exception details. |
|
You can use some fancy HTML 5 tags to have collapsible bits if you want. <details>
<summary>Collapsible panel!</summary>
This is the detail that is hidden by the panel.
</details>That renders as: Collapsible panel!This is the detail that is hidden by the panel.I don't think it makes the emails shorter, but the rendered view gets tidier. |
|
Will do, doing so also for the complete output. Will just hide the error instead! |
|
90% of the time all we'll care about is the summary table from each benchmark. The full verbose output makes it harder to compare. Any way to only print the result table of each unless we explicitly ask for it to be more verbose? |
You want the full output in the collapse only if requested? |
|
Could do, but I'd also be happy with a Basically, I'm not too fussed about the exact format, provided the two result tables are visually adjacent when viewed in GitHub. |
|
Sure, can do so, hopefully the upcoming weekend. For errors it will look like that for now: |
|
Cool, thanks. 😀 I'll take a look at the updates to this PR tomorrow and get it merged in. |
@tillig would be great if you could clone and add the repositories to the org, so that we can collect issues/feature requests there |
|
Yup, I'm a little under the gun at work so this isn't going to happen today. |
|
No worries, tyt. |
There was a problem hiding this comment.
This feels pretty good now; I'm going to merge it. Thanks @alsami for sticking with this one through all the changes!
Part of #1164, bringing back performance improvements
ResolveRequestContexttoDefaultResolveRequestContextResolveRequestContextBasetoResolveRequestContextMockResolveRequestContexttoResolveRequestContextStubMockLifetimeScopetoLifetimeScopeStubResolveRequestContextSub