Removed OfType usage. Less allocations and much faster

[ShreyasJejurkar]

The following benchmark test shows usage of OfType extension method can be slower and allocates double memory as compared to good old foreach even on primitive types like string and int.

This PR removes one of the usages of OfType in the method, which gets called by many other extension methods internally like AddMvcCore and AddControllers etc.

    [MemoryDiagnoser]
    public class OfTypeBenchMark
    {
        private static IList<Type> types = new List<Type> { typeof(string), typeof(int) };

        [Benchmark]
        public bool WithOfTypeExtensionMethod()
        {
            return types.OfType<string>().Any();
        }

        [Benchmark]
        public bool WithoutOfType()
        {
            var result = false;

            foreach(var type in types)
            {
                if(type is string)
                {
                    result = true;
                    break;
                }
            }

            return result;
        }
    }

|                    Method |      Mean |    Error |   StdDev |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|-------------------------- |----------:|---------:|---------:|-------:|------:|------:|----------:|
| WithOfTypeExtensionMethod | 106.78 ns | 0.356 ns | 0.316 ns | 0.0612 |     - |     - |      96 B |
|             WithoutOfType |  41.58 ns | 0.117 ns | 0.104 ns | 0.0255 |     - |     - |      40 B |

Contributes to dotnet/runtime#44598

[En3Tho]

I don't think it's necessary to replace easily readable Linq with manual foreach in methods that run only once. I doubt it will bring any noticeable difference. I believe that issue you've linked is about optimizing core things, but I might be wrong.

[ShreyasJejurkar]

LINQ brings readability, but it lowers the performance! And this method is get's called at startup, and we want to make startup as fast as possible!
Some more info -> dotnet/runtime#45060

[AmrAlSayed0]

You're benchmark is wrong. You're list has objects of type Type and you're trying to cast it to string in the first test.

The second test just checks for equality which is not the same thing.

[sharwell]

You should be able to get exactly the same performance from this:

Suggested change

	var controllerFeaturePresent = false;
	foreach(var feature in manager.FeatureProviders)
	{
	if(feature is ControllerFeatureProvider)
	{
	controllerFeaturePresent = true;
	break;
	}
	}
	if(!controllerFeaturePresent)
	if(!manager.FeatureProviders.Any(provider => provider is ControllerFeatureProvider))

[stephentoub]

exactly the same performance

It won't be "exactly" the same: every element would now involve a delegate invocation.

But I don't think that matters here. I agree with Sam we should keep this simple and that this is a perfectly reasonable place to use Enumerable.Any(func). I would change my tune if this were part of a default Blazor wasm app and was the last thing keeping Enumerable.Any alive in such an app and we could otherwise trim out Enumerable.Any, but it's not.

[javiercn]

I agree with @stephentoub that the code is fine as it is and we don't need to change it unless we see significant performance benefits. I believe there should be other areas we should look at first before going for these micro optimizations.

[ShreyasJejurkar]

My intension is to remove out slow things that impacts startup. I know resulting code is not quite readable, but also it's not that much hard to understand! 😄

[javiercn]

My intension is to remove out slow things that impacts startup.

What's the improvement over the overall startup time?

[sharwell]

@javiercn would you agree that the alternative form I proposed is equally readable to the original?

[javiercn]

@javiercn would you agree that the alternative form I proposed is equally readable to the original?

Yes, I'm not really complaining about any of these being less readable, I'm suggesting that maybe is not worth changing given that I don't have a clear idea of how much improvement this brings to the overall startup experience (I see from the issue linked that there is a general effort to improve this area in general).

I'm just pointing out that I don't know the impact this has in total startup time, but I suspect is negligible, and at that point, is it worth changing the code?

The risk here is the chance of introducing a small/subtle behavior change/bug, if we are taking that risk, at least we should make sure is worth it.

Again, I'm happy if we take this PR in whatever shape provided the implementation remains equivalent, but for future reference we should at least establish a minimal threshold to determine what's worth optimizing.

Does that make sense?

[sharwell]

Does that make sense?

Certainly, was just getting clarification on the reason for wanting more data. 👍

@MCCshreyas if you want to capture more data about this, one of the easiest things to capture is a count of the number of times this method is called during the startup process. If the count varies according to the program configuration, then a set of minimum/expected/maximum counts would be particularly helpful.

[pranavkm]

f you want to capture more data about this, one of the easiest things to capture is a count of the number of times this method is called during the startup process

In the templates, it's usually invoked once.

[pranavkm]

@MCCshreyas thanks for your PR. I think we need to be able to notice measurable startup improvements for changes like this. Unfortunately microbenchmarks might not necessarily demonstrate this. I'm closing this. If you get the opportunity to measure startup perf (https://github.com/dotnet/crank has features that can let you do this), we'd be happy to reconsider this change.