improve performance of ValueRetrievers

[bollhals]

Hey

In my project I am using the TableHelperExtensionMethods quite extensively and my analysis showed that they're not the fastest, so I wanted to have a look whether I can improve things a bit.
I started by improving the performance of the ValueRetrievers to get a feeling and to see if this is something you want to include.

Regarding the changes in this PR.

• I've tried to keep the public interface as is.
• The functionality should be the same as before.
• I've restructured / simplified the tests a bit, but kept the test data and extended with a few new ones for missing cases.

Let me know what you think.
Cheers

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

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.

[SabotageAndi]

Thanks for the PR. It's quite big so we will need some time to review it.

It would be interesting how much the performance was improved with these changes. Could you provide some numbers for us?

[bollhals]

Thanks for the PR. It's quite big so we will need some time to review it.

It would be interesting how much the performance was improved with these changes. Could you provide some numbers for us?
Sure, no problem, just comment if you have any questions or feedback.

There are two possible ways to get some numbers.

1. Via profiling the existing unit tests.
2. Via a new benchmarking project and usage of BenchmarkDotNet.

Regarding 1, it's difficult to get proper numbers as the general test execute too quickly (< 2ms). I can give you one example of the following test, as this one takes some time to execute:
ListRetrieverTests.Retrieves_value_enumeration_from_comma_separated_list (scoped to the Retrieve Method, which is the relevant method to test)
Old 172 ms:

New 148 ms:

~24 ms less, ~14% less time spent

Regarding 2, if you're open for it, I can also set up a new project and throw in some benchmarks so we can see it more closely.

[bollhals]

Maybe this test also highlights some benefits:
CreateInstanceHelperMethodTests.Can_create_an_instance_with_a_constructor:
Old 46 ms:

New 38ms:

~8 ms less, ~18% less time spent

Keep in mind though, profiling tests always means we're only seeing full initialization time (so including JIT, static ctors, etc)
Some of my changes target the init time, so the benefits will be less if the same function gets called multiple times / during a full test run.

[304NotModified]

While I'm not very impressed with the performance improvement, I really like that a lot of code has been deleted (I'm serious). Less code = less maintenance and less bugs. Also I think it's really nice that there are many tests cases added.

[SabotageAndi]

Sorry, it took so long for reviewing this PR. Thanks for doing this work. Not only improves the performance, but I think it makes it also easier to maintain the ValueRetrievers in the future because there is now only a single implementation per type.

Sadly some other changes in the past so there are some merge conflicts.
Could you fix them? Then I will merge this PR. Everything else is fine.

[bollhals]

Sure no problem. I'll try to get it done sometime this week.

[304NotModified]

I would also add that the nullable retrievers aren't needed anymore :)

[bollhals]

Done.
I think I got all the testcases you've introduced in #1941 But would you mind doublechecking?

[304NotModified]

I don't like bringing bad news, but this is a breaking change.

@SabotageAndi is this allowed?

Another options are:

• make them as obsolete (and maybe new folder), and remove them in a new major version (V4), or
• create a new package with the old classes, e.g. TechTalk.SpecFlow.Assist.ValueRetrievers.Legacy or TechTalk.SpecFlow.Assist.ValueRetrievers.Nullable

[SabotageAndi]

As these changes go into the next minor version (3.3), I have no big problems with some breaking changes. But it would be good to list this in the API Changes of the changelog.

[bollhals]

I've updated the changelog with an entry for the breaking changes of the classes.

[godrose]

Hooray!

On Fri, 8 May 2020 at 15:16 bollhals ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In changelog.txt <#1928 (comment)>: > @@ -2,6 +2,7 @@ Changes since 3.1.89 Fixes: + Update the default C# skeleton template to use context injection instead of the deprecated ScenarioContext.Current ++ Improved performance of ValueRetrievers Done. I think I got all the testcases you've introduced in #1941 <#1941> But would you mind doublechecking? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1928 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3OIIZFWVKM374NJUF3TC3RQPZ3FANCNFSM4LVYHGLQ> .

-- Best Regards, Gennady Verdel

[SabotageAndi]

I will try to find some time on Monday/Tuesday to review the PR.

[SabotageAndi]

Thanks for fixing the merge conflicts and for your contribution to SpecFlow.

If you would like us to send you some SpecFlow stickers as a thank you, please fill out the form here.