remove linq from hot paths

[bollhals]

This PR contains 2 main changes:

• sealing types in ContextManager and removing LINQ from IsEmpty (!stack.Any() is equal to stack.Count == 0, but much faster and inlineable)
• reducing allocation and improving performance in StepContext Ctor by removing LINQ

Types of changes

• Bug fix (non-breaking change which fixes an issue).
• New feature (non-breaking change which adds functionality).
• Breaking change (fix or feature that would cause existing functionality to not work as expected).
• Performance improvement
• Refactoring (so no functional change)
• Other (docs, build config, etc)

Checklist:

• I've added tests for my code. (most of the time mandatory)
• I have added an entry to the changelog. (mandatory)
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

Comments

• Do you want proof of every change?
• I like to add #nullable enable to every class, is this fine for you? (see Nullable Reference Types)

[SabotageAndi]

Could you give us some numbers of performance increase of this change?
I am sure the new code is faster, but I would like to know how much. We have to balance performance and maintainability.

[SabotageAndi]

About #nullable enable: We got the build down to no warnings and I would like to keep it that way. I am not sure if adding this statement to new classes would trigger a lot of warnings. Fixing these would probably hide then the real work of the PR.

Don't understand me wrong, I want that SpecFlow has nullable reference types. I simply had not yet enough time to think about it how to get started with it.

[bollhals]

About #nullable enable: We got the build down to no warnings and I would like to keep it that way. I am not sure if adding this statement to new classes would trigger a lot of warnings. Fixing these would probably hide then the real work of the PR.

Don't understand me wrong, I want that SpecFlow has nullable reference types. I simply had not yet enough time to think about it how to get started with it.

You only get warnings where you have it enabled. Meaning even if you use a class that has it enabled, you won't get warnings if your class has not enabled it. And if you enable it, you should fix the warning. This is how I usually do it for other projects. This means the whole codebase gets continuesly nullableaware.

[bollhals]

Could you give us some numbers of performance increase of this change?
I am sure the new code is faster, but I would like to know how much. We have to balance performance and maintainability.

As always I only have the profiler output and not some proper microbenchmarks. (This would need to be added to the solution).
The benefits in terms of performance are hard to quantify without this. but I can give you some context:
Before / After


You see the get_Instance call under the profiler is twice as fast. When I quickly tested it in a microbenchmark, the Stack.Any took ~16ns while the stack.Count == 0 took only 1ns (16x faster)

One other thing to consider here is that Any() allocates in certain conditions an Enumerator (40 Bytes) on the heap, which then the GC needs to cleanup. So changing it to ".Count == 0" also does not allocate.

Similar for the other change.
Before / After

But it also removes the Distinct usage, which resulted in defered execution of it which then again allocated even more bytes.

(See the two Contains calls, one to Linq.Enumerable.Contains (Distinct case) and one to SZArrayHelper (Array case), the former one will on each evaluation perform the distinct result of the two arrays (tags for feature / scenario) by allocating a hashset to do so)

[SabotageAndi]

@bollhals The team had a look at this PR and we discussed it internally if we want to merge it.
We came to the decision to not merge it. We have to maintain this code in the end and this time it is a too big decrease in maintainability for us.

[bollhals]

@bollhals The team had a look at this PR and we discussed it internally if we want to merge it.
We came to the decision to not merge it. We have to maintain this code in the end and this time it is a too big decrease in maintainability for us.

I understand your decision.

Since maintainability is often subjective, would you be able to shed a bit of light on what you'd consider too big of a decrease and what not? (Simplifies other existing & future PR as well as possibly getting some of the improvements of this PR in another acceptable form)

Examples:

public bool IsEmpty => !instances.Any();

vs

public bool IsEmpty => instances.Count == 0;

or

tags.Concat(scenarioInfo.Tags).Distinct();

vs

        private static IEnumerable<string> CombineTags(string[] featureTags, string[] scenarioTags)
        {
            var result = new HashSet<string>(featureTags);
            foreach (string tag in scenarioTags)
            {
                result.Add(tag);
            }

            return result;
        }

or is it the larger size of the code / chaining ifs?

[SabotageAndi]

In this case, it was the amount of new code (from 10 to 50 lines) and the 4 level of ifs.

From your examples, the first case is totally fine.

The second case would be ok but depends on the usage of it.

[bollhals]

Thanks for the explanation. I'll see what's possible to squeeze out👍🏼