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

feature: splat init checks for pre registrations #360

Merged
merged 16 commits into from
Jul 27, 2019
Merged

Conversation

dpvreony
Copy link
Member

@dpvreony dpvreony commented Jul 16, 2019

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
@codecov
Copy link

codecov bot commented Jul 17, 2019

Codecov Report

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

Impacted file tree graph

@@            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.

@dpvreony dpvreony changed the title Feature: splat init checks for pre registrations WIP Feature: splat init checks for pre registrations Jul 17, 2019
@glennawatson
Copy link
Contributor

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

src/Splat/ServiceLocation/ModernDependencyResolver.cs Outdated Show resolved Hide resolved
src/Splat/ServiceLocation/ModernDependencyResolver.cs Outdated Show resolved Hide resolved
src/Splat/ServiceLocation/ModernDependencyResolver.cs Outdated Show resolved Hide resolved
src/Splat/ServiceLocation/ModernDependencyResolver.cs Outdated Show resolved Hide resolved
src/Splat/ServiceLocation/ModernDependencyResolver.cs Outdated Show resolved Hide resolved
src/Splat/ServiceLocation/ModernDependencyResolver.cs Outdated Show resolved Hide resolved
@glennawatson
Copy link
Contributor

glennawatson commented Jul 18, 2019

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
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
Copy link
Contributor

glennawatson commented Jul 18, 2019

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
Copy link
Contributor

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
Copy link
Member Author

dpvreony commented Jul 18, 2019

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
Copy link
Contributor

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

@glennawatson
Copy link
Contributor

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

@glennawatson
Copy link
Contributor

glennawatson commented Jul 21, 2019

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

@reactiveui reactiveui deleted a comment from todo bot Jul 22, 2019
@reactiveui reactiveui deleted a comment from todo bot Jul 22, 2019
@reactiveui reactiveui deleted a comment from todo bot Jul 22, 2019
@reactiveui reactiveui deleted a comment from todo bot Jul 22, 2019
@dpvreony
Copy link
Member Author

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

@glennawatson
Copy link
Contributor

got much left to do on this one @dpvreony ?

@glennawatson glennawatson changed the title WIP Feature: splat init checks for pre registrations feature: splat init checks for pre registrations Jul 27, 2019
@glennawatson glennawatson merged commit ac6c329 into master Jul 27, 2019
@delete-merged-branch delete-merged-branch bot deleted the bug/331 branch July 27, 2019 13:14
@lock lock bot locked and limited conversation to collaborators Oct 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using DryIoC with an ILogger
2 participants