feature: splat init checks for pre registrations #360

dpvreony · 2019-07-16T08:02:23Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Resolves #331
I've changed the splat init logic to check if the resolver it is being hooked up to has already got registrations to types we're interested in (LogManager, Logger, BitmapLoader)

What is the current behavior? (You can also link to an open issue here)
depending on the nuance of the container involved. logging would be overridden or appended with no guaranteed order of retrieval.

What is the new behavior (if this is a feature change)?
tried to apply some consistency between dryioc and moderndependencyresolver. developers still have control within dryioc etc to get undesired behaviour. added documentation to that effect.

What might this PR break?
I've added a HasRegistration method call to the Mutable Resolver interface

I've changed DryIoc to replace registrations instead of append - can someone sense check this please :)

Please check if the PR fulfills these requirements

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

checks to see if a resolver already has registrations for types we're interested in

src/Splat/ServiceLocation/ModernDependencyResolver.cs

src/Splat/ServiceLocation/DependencyResolverMixins.cs

src/Splat.SimpleInjector/SimpleInjectorDependencyResolver.cs

src/Splat.DryIoc/DryIocDependencyResolver.cs

…n check

codecov · 2019-07-17T09:54:23Z

Codecov Report

❗ No coverage uploaded for pull request base (master@305c589). Click here to learn what that means.
The diff coverage is 83.67%.

@@            Coverage Diff            @@
##             master     #360   +/-   ##
=========================================
  Coverage          ?   71.33%           
=========================================
  Files             ?       66           
  Lines             ?     3663           
  Branches          ?      333           
=========================================
  Hits              ?     2613           
  Misses            ?     1004           
  Partials          ?       46

Impacted Files	Coverage Δ
.../Splat/ServiceLocation/DependencyResolverMixins.cs	`81.81% <ø> (ø)`
...rc/Splat/ServiceLocation/FuncDependencyResolver.cs	`30.9% <0%> (ø)`
...t/ServiceLocation/ServiceLocationInitialization.cs	`100% <100%> (ø)`
...SimpleInjector/SimpleInjectorDependencyResolver.cs	`35.29% <100%> (ø)`
src/Splat.DryIoc/DryIocDependencyResolver.cs	`57.57% <100%> (ø)`
src/Splat.Autofac/AutofacDependencyResolver.cs	`78.4% <80.18%> (ø)`
.../Splat/ServiceLocation/ModernDependencyResolver.cs	`52.23% <94.11%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 305c589...c078dd6. Read the comment docs.

glennawatson · 2019-07-17T10:06:49Z

I been slowly merging everything over to valuetuple format over tuple. Usually has greater self documentation also performance advantages.

src/Splat/ServiceLocation/DependencyResolverMixins.cs

src/Splat/ServiceLocation/ModernDependencyResolver.cs

glennawatson · 2019-07-18T03:37:56Z

This is a good overview and how to use the new value tuple shorthand https://blogs.msdn.microsoft.com/mazhou/2017/05/26/c-7-series-part-1-value-tuples/

Mark Zhou's Tech Blog
C# 7 Series, Part 1: Value Tuples
Starting today I will start a new C# 7 series, to introduce new language features of C# 7+ features. Please note that I am not saying C# 7.0, I am saying C# 7 plus, because there will be minor language versions (like 7.1, 7.2) that will bring new features in steps (thanks to Roslyn!) such...

glennawatson · 2019-07-18T06:58:43Z

I just did a benchmark of using the ValueTuple directly vs GetKey()

|         Method |     Mean |     Error |    StdDev |   Median | Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------------- |---------:|----------:|----------:|---------:|------:|------:|------:|----------:|
|    TestWithGet | 1.198 ms | 0.0423 ms | 0.1201 ms | 1.164 ms |     - |     - |     - |         - |
| TestWithoutGet | 1.179 ms | 0.0344 ms | 0.0954 ms | 1.156 ms |     - |     - |     - |         - |

My test was simple

        [Benchmark]
        public void TestWithGet()
        {
            for (int i = 0; i < NumberCases; ++i)
            {
                var key = Get(ElementsNames1[i], ElementsNames2[i]);

                var element = _indexToElements[key];

                outputElements.Add(element);
            }
        }

        [Benchmark]
        public void TestWithoutGet()
        {
            for (int i = 0; i < NumberCases; ++i)
            {
                var key = (ElementsNames1[i], ElementsNames2[i]);

                var element = _indexToElements[key];

                outputElements.Add(element);
            }
        }

Number of test cases was 10,000

That's with randomly allocated keys etc.

So that confirms the way you are doing it is fractionally slower if anything meaningful.

glennawatson · 2019-07-18T07:35:11Z

Also just did this for curiosity, added the following benchmark

        [Benchmark]
        public void TestWithTupleGet()
        {
            for (int i = 0; i < NumberCases; ++i)
            {
                var key = Tuple.Create(ElementsNames1[i], ElementsNames2[i]);

                var element = _indexToTupleElements[key];

                outputElements.Add(element);
            }
        }

|           Method |     Mean |     Error |    StdDev |   Median | Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------------- |---------:|----------:|----------:|---------:|------:|------:|------:|----------:|
|      TestWithGet | 1.297 ms | 0.0626 ms | 0.1754 ms | 1.240 ms |     - |     - |     - |         - |
|   TestWithoutGet | 1.361 ms | 0.0837 ms | 0.2401 ms | 1.278 ms |     - |     - |     - |         - |
| TestWithTupleGet | 2.212 ms | 0.0883 ms | 0.2371 ms | 2.194 ms |     - |     - |     - |  320000 B |

dpvreony · 2019-07-18T08:11:31Z

Also just did this for curiosity, added the following benchmark
[Benchmark]
public void TestWithTupleGet()
{
for (int i = 0; i < NumberCases; ++i)
{
var key = Tuple.Create(ElementsNames1[i], ElementsNames2[i]);
            var element = _indexToTupleElements[key];

            outputElements.Add(element);
        }
    }
Method Mean Error StdDev Median Gen 0 Gen 1 Gen 2 Allocated

TestWithGet 1.297 ms 0.0626 ms 0.1754 ms 1.240 ms - - - -

TestWithoutGet 1.361 ms 0.0837 ms 0.2401 ms 1.278 ms - - - -

TestWithTupleGet 2.212 ms 0.0883 ms 0.2371 ms 2.194 ms - - - 320000 B

With and Without both inside the margin for error? And the result reversed 2nd time around. I can decorate with MethodImplOptions.AggressiveInlining? (if it's not already inlining?) or put it back. I've no preference :)

glennawatson · 2019-07-18T08:12:26Z

The error increased on the second run. Which means my computer was probably under greater strain at the time.

glennawatson · 2019-07-18T08:12:59Z

I think that shows how little margin is in it that the Get() approach is fine, but getting rid of Tuple is validated.

glennawatson · 2019-07-21T12:05:42Z

Might as well bump the minor version by one by incrementing the second number in version.json in the root directory for this one.

dpvreony · 2019-07-22T21:29:35Z

be nice if the todo comments were done as closable review, deleted them for clarity on this PR

glennawatson · 2019-07-27T09:18:39Z

got much left to do on this one @dpvreony ?

dpvreony added 2 commits July 16, 2019 08:46

#331 check for pre init components

650dc7f

checks to see if a resolver already has registrations for types we're interested in

doc warning about direct usage of ioc container

b8a847b

dpvreony added Breaking change Improvement developer-experience labels Jul 16, 2019

dpvreony requested a review from a team July 16, 2019 08:02

glennawatson reviewed Jul 16, 2019

View reviewed changes

src/Splat/ServiceLocation/ModernDependencyResolver.cs Outdated Show resolved Hide resolved

glennawatson reviewed Jul 16, 2019

View reviewed changes

src/Splat/ServiceLocation/DependencyResolverMixins.cs Outdated Show resolved Hide resolved

glennawatson reviewed Jul 16, 2019

View reviewed changes

src/Splat.SimpleInjector/SimpleInjectorDependencyResolver.cs Show resolved Hide resolved

glennawatson reviewed Jul 16, 2019

View reviewed changes

src/Splat.DryIoc/DryIocDependencyResolver.cs Outdated Show resolved Hide resolved

#360 removed use of linq in modern dependency resolver hasregistratio…

83d8219

…n check

dpvreony changed the title ~~Feature: splat init checks for pre registrations~~ WIP Feature: splat init checks for pre registrations Jul 17, 2019

dpvreony added 3 commits July 17, 2019 11:53

autofac logmanager resolving tests

47b436a

modern dependency resolver now using valuetuple

0d99e24

dryioc now uses last service registration

b31bf95

dpvreony commented Jul 17, 2019

View reviewed changes

src/Splat/ServiceLocation/DependencyResolverMixins.cs Outdated Show resolved Hide resolved

fixed IndexOutOfRangeException in UnregisterCurrent

9e1535c

glennawatson reviewed Jul 18, 2019

View reviewed changes

src/Splat/ServiceLocation/ModernDependencyResolver.cs Outdated Show resolved Hide resolved

glennawatson requested changes Jul 18, 2019

View reviewed changes

Merge branch 'master' into bug/331

57cb3dd

dpvreony added 2 commits July 18, 2019 07:18

modern dep resolver - refactor getkey and use c# 8 syntax

f74d490

split out the resolver init

f6d025e

dpvreony added 2 commits July 18, 2019 09:15

initial work on autofac unregister

32f622a

autofac unregister logic

4451876

dpvreony mentioned this pull request Jul 21, 2019

Housekeeping: Locator GetAllRegistrations against different DI #363

Open

more tests, fixed autofac remove not removing anything

24b9fea

dpvreony added 3 commits July 21, 2019 15:38

Merge branch 'master' into bug/331

9212318

v8.1 bump

f164189

autofac lock blocks

c078dd6

reactiveui deleted a comment from todo bot Jul 22, 2019

glennawatson approved these changes Jul 27, 2019

View reviewed changes

glennawatson changed the title ~~WIP Feature: splat init checks for pre registrations~~ feature: splat init checks for pre registrations Jul 27, 2019

glennawatson merged commit ac6c329 into master Jul 27, 2019

delete-merged-branch bot deleted the bug/331 branch July 27, 2019 13:14

lock bot locked and limited conversation to collaborators Oct 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: splat init checks for pre registrations #360

feature: splat init checks for pre registrations #360

dpvreony commented Jul 16, 2019 •

edited

Loading

codecov bot commented Jul 17, 2019 •

edited

Loading

glennawatson commented Jul 17, 2019

glennawatson commented Jul 18, 2019 •

edited by unfurl-links bot

Loading

glennawatson commented Jul 18, 2019 •

edited

Loading

glennawatson commented Jul 18, 2019

dpvreony commented Jul 18, 2019 •

edited

Loading

glennawatson commented Jul 18, 2019

glennawatson commented Jul 18, 2019

glennawatson commented Jul 21, 2019 •

edited

Loading

dpvreony commented Jul 22, 2019

glennawatson commented Jul 27, 2019

feature: splat init checks for pre registrations #360

feature: splat init checks for pre registrations #360

Conversation

dpvreony commented Jul 16, 2019 • edited Loading

codecov bot commented Jul 17, 2019 • edited Loading

Codecov Report

glennawatson commented Jul 17, 2019

glennawatson commented Jul 18, 2019 • edited by unfurl-links bot Loading

glennawatson commented Jul 18, 2019 • edited Loading

glennawatson commented Jul 18, 2019

dpvreony commented Jul 18, 2019 • edited Loading

glennawatson commented Jul 18, 2019

glennawatson commented Jul 18, 2019

glennawatson commented Jul 21, 2019 • edited Loading

dpvreony commented Jul 22, 2019

glennawatson commented Jul 27, 2019

dpvreony commented Jul 16, 2019 •

edited

Loading

codecov bot commented Jul 17, 2019 •

edited

Loading

glennawatson commented Jul 18, 2019 •

edited by unfurl-links bot

Loading

glennawatson commented Jul 18, 2019 •

edited

Loading

dpvreony commented Jul 18, 2019 •

edited

Loading

glennawatson commented Jul 21, 2019 •

edited

Loading