Skip to content
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

Circular Dependency Changes #1148

Merged
merged 5 commits into from Jun 10, 2020
Merged

Conversation

alistairjevans
Copy link
Member

I know how much @tillig likes looking at a PR every day, so.....

I've made two changes to the way circular dependencies are detected.

Max Depth

The first is to the 'max depth' constraint, which currently has a limit of 50. Based on @VonOgre's suggestion of RuntimeHelpers.EnsureSufficientExecutionStack() in #575 (👍), there is another method, RuntimeHelpers.TryEnsureSufficientExecutionStack(). On netstandard2.1, when we hit the normal 50 limit, rather than stop immediately, we keep going until TryEnsureSufficientExecutionStack returns false.

These methods do not exist prior to netstandard2.1, so the existing '50' limit is applied.

This should please anyone with insanely deep service graphs; on my testing, I hit 891 resolve requests before it stopped.

Side note: tracing does still work on in this new style of stack-checking. It generates a significant amount of text though...

Request Stack Change - Depth and Segments

We talked already about pre-allocating the request stack in #1127 ; that has been done (default size of 16). That's actually pretty deep for 90% of users I'd guess; it'll then double each resize after that.

In addition, I wanted to fix an annoying problem with decorators, where they had to reach out and execute a top-level ResolveOperation instead of just resolving the decorator in the same IComponentContext resulting in a whole other allocation of the ResolveOperation, and generally some sad code. They did this because they needed a fresh circular dependency detection stack.

To counter this, I've replaced the use of Stack<T> with a new SegmentedStack<T>. This type behaves almost exactly like a regular stack, except it allows you define a point in the stack to start a new 'segment'. The enumerator for the stack then only operates within the segment.

In this way we can mark a point in the request stack beyond which we do not look for any further dependencies.

In the DecoratorMiddleware:

using (context.Operation.EnterNewDependencyDetectionBlock())
{
    var decoratedInstance = context.ResolveComponent(resolveRequest);

    context.Instance = decoratedInstance;

    context.DecoratorContext = context.DecoratorContext.UpdateContext(decoratedInstance);
}

This gives us some memory and performance gains on resolving decorators (KeylessGenericBenchmark):

v6

Method repetitions Mean Error Gen 0 Gen 1 Gen 2 Allocated
Baseline ? 540.1 ns 5.71 ns 0.5102 - - 2.09 KB
ResolveEnumerableT 1 3,931.6 ns 40.18 ns 1.7395 - - 7.14 KB
ResolveT 1 1,973.3 ns 24.02 ns 1.0872 - - 4.45 KB
ResolveEnumerableT 2 7,380.6 ns 54.92 ns 2.9831 - - 12.2 KB
ResolveT 2 3,381.3 ns 24.34 ns 1.6670 - - 6.82 KB
ResolveEnumerableT 3 10,589.0 ns 76.76 ns 4.2114 - - 17.25 KB
ResolveT 3 4,688.6 ns 21.50 ns 2.2430 - - 9.19 KB

request-stack

Method repetitions Mean Error Gen 0 Gen 1 Gen 2 Allocated
Baseline ? 537.8 ns 4.15 ns 0.5102 - - 2.09 KB
ResolveEnumerableT 1 3,841.2 ns 25.83 ns 1.5793 - - 6.47 KB
ResolveT 1 1,892.1 ns 10.53 ns 1.0090 - - 4.13 KB
ResolveEnumerableT 2 7,234.6 ns 25.49 ns 2.6550 - - 10.85 KB
ResolveT 2 3,091.5 ns 19.88 ns 1.5068 - - 6.16 KB
ResolveEnumerableT 3 10,198.3 ns 131.48 ns 3.7231 - - 15.23 KB
ResolveT 3 4,326.6 ns 33.11 ns 2.0065 - - 8.2 KB

Define a new SegmentedStack<T> that allows us maintain segments of the request stack, meaning we can resolve decorators without allocating an entirely new operation.
…e passed our typicaly max resolve depth, and only throw once it fails.
@tillig
Copy link
Member

tillig commented Jun 9, 2020

This was definitely a hole in my life, to be sure.

Looks like some good improvements, though, so... I guess I'll dig in. 😆

Copy link
Member

@tillig tillig left a comment

Choose a reason for hiding this comment

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

I love the smell of a PR in the morning. Smells like... victory.

Couple minor questions. Otherwise, 🤠 YAHOOOOOOO

Copy link
Member

@tillig tillig left a comment

Choose a reason for hiding this comment

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

🥇

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

Successfully merging this pull request may close these issues.

None yet

2 participants