Closed Yortw closed 3 years ago
@gkarabin care to review this as well :)
It's on my weekend reading list! I will have a hard time looking at it for long for a few days, but I will review it as I can.
I'm a few commits into the review at this point. So far things look excellent from a functional point of view, but I need to do some of the commits on a computer (GitHub is not great on iPadOS).
One general comment is that there's quite a bit of unnecessary white space changing to this point. It's nothing that will prevent completing the review, but it does slow things down checking for possible differences. Whether or not you want to rebase and adjust the commits is up to you.
My apologies for the whitespace changes, I tried hard not to reformat things just because but my IDE obviously has different defaults configured and there wasn't an editor config setup to enforce consistency so I may have inadvertently adjusted portions I changed trying to get it look right locally, forgetting that was going to cause an issue. This is only my second(?) PR to an open source project so it's likely I've made a few errors like this, sorry. I appreciate everyone taking the time to review. If there's someway I can tidy up the whitespace for you I'd be happy to do it but may need a hand guided through the process... I don't normally use git and rebasing is something I'm unfamiliar with, and I'm not sure of exactly how to edit the commits to get the desired outcome either.
My apologies for the whitespace changes, I tried hard not to reformat things just because but my IDE obviously has different defaults configured and there wasn't an editor config setup to enforce consistency so I may have inadvertently adjusted portions I changed trying to get it look right locally, forgetting that was going to cause an issue. This is only my second(?) PR to an open source project so it's likely I've made a few errors like this, sorry. I appreciate everyone taking the time to review. If there's someway I can tidy up the whitespace for you I'd be happy to do it but may need a hand guided through the process... I don't normally use git and rebasing is something I'm unfamiliar with, and I'm not sure of exactly how to edit the commits to get the desired outcome either.
Rebasing (interactively) is probably a tricky thing to ask for if you aren't familiar with the process. My favorite explanation is here (see "Splitting a Commit"). You can use the "edit" command on a commit and then use an editor in which you can see and remove the whitespace changes. Then you re-commit the change (pasting the previous commit notice), and let the rebase continue.
Sometimes there are "merge" conflicts reapplying subsequent commits. Handling them can be very confusing. If you run into many of them on your first rebase you may well give up. Once you get the hang of it, it's manageable, but it is an art.
If you choose to try it out, if I were you, I would create a new branch off of your existing work for experimenting with the process. If you want to be extra careful, you might even choose to do it from another clone of the TinyIoC repo, to avoid steps that could mess up your "good" repo. I rebase commits most every day without issues or extra safety measures, but when I was learning I occasionally made impressive mistakes that left me hunting for "the old version" in dangling commits so I could start over.
Thanks for the PR!
What
These changes improve performance of the TinyIocContainer, mostly for the Resolve method calls but also some minor improvements to AutoRegister. Most (but not all) performance improvements to Resolve follow the existing patterns (for compiled expressions etc) where a small performance penalty may be paid on the first call to resolve a registration due to caching/compilation etc. but subsequent resolutions of the same registration will be faster. Also includes a new project in the solution (under the tests folder) using BenchmarkDotNet to confirm that any changes made are actually an improvement.
Also while I'm here I would like to thank GrumpyDev and everyone else who works on this project. TinyIoC is my go to IoC for most projects, it's simple, fast, light weight and just works. It's also easy to debug through when I screw up registrations etc. I love it! Thank you :)
Why
TinyIoC has always been "fast enough" for me and I've never worried about it's performance before. However I'm currently trying to get a Xamarin Forms app to work on a specific piece of hardware running a custom Android OS and performance of the app is abysmal. I am therefore trying to optimize every part of the app and although I don't think TinyIoC is a significant part of my problem it seemed like there might be some easy wins here so I put in some effort to see if I could make it go faster.
Possibly breaking change
All of the changes to improve the performance of the Resolve method are intended to be 'safe' and 'non-breaking'. All existing unit tests still pass (no new tests added).
However commit 10a2be1de102ce7fcd7a72f07e5c496c5afbe946 for improving the AutoRegister method has a potentially breaking change where it doesn't register some types it used to. Specifically types that are nested and private are no longer registered. While this is potentially a breaking change I think it is a good one for several reasons:
There are a few options here;
Performance Improvements
Below are the results of the improvements made, as measured using Benchmark dot net. The size of the improvement varies by platform and use case, but as you can see in some cases the improvements are signficant.