Allocation cleanup

[rossgrambo]

Why this PR?

We noticed we were allocating a lot of memory in v4 compared to v3. This PR resolves many instances of extra allocations or memory and resolves some issues that existed in v3 as well.

Now- the allocations are just the EvaluationEvent object (which could be improved more for the boolean and missing defintion case), async closures, and enumerators only when needed.

Additionally-

• Resolves some spelling issues
• Adds a pull request template
• Bumps vulnerable packages in examples and test projects

Visible Changes

No changes are visible to developers using the library. They may notice allocation wins.

[rossgrambo]

@avanigupta @zhiyuanliang-ms @zhenlan This is blocking v4 release If you have time to review!

[zhiyuanliang-ms]

Can we use the tool like benchmark .net to verifiy this change can improve the performance?

[avanigupta]

Can we create an issue to track this? It doesn't need to block v4 release.

[rossgrambo]

Created issue: #512

In the meantime- I created a repo to run BenchmarkDotNet tests against multiple versions of this library. Here's a link to the results/repo (Same results as the screenshot shared in another comment): https://github.com/rossgrambo/FeatureManagementBenchmark/blob/main/BenchmarkDotNet.Artifacts/results/FeatureManagementBenchmark.Benchmarks-report-github.md

[zhiyuanliang-ms]

@rossgrambo Could please explain why storing task in the dictionary can save memory?

[rossgrambo]

The single line that caused a developer to write in was:

return Task.FromResult(
                _definitions.GetOrAdd(
                    featureName,
                    (_) => GetMicrosoftSchemaFeatureDefinition(featureName) ?? GetDotnetSchemaFeatureDefinition(featureName)));

The problem with this function is that the lambda/delegate/func syntax of:

(_) => ...

Creates a Func. Although it's not called in the case we hit the cache- it still allocates memory for the func every time this line is hit. This is because it happens before the GetOrAdd method is called actually called, as the parameters are resolved first before the method can be called.

This line also uses Task.FromResult. This creates a unique Task object on each request- even when the item is found in cache. By adjusting the cache to include the Task, we're able to keep a 1:1 relationship between Task and FeatureDefinition- rather than a 1:1 between Task and number of calls to IsEnabledAsync().

[zhiyuanliang-ms]

Discussed offline with @rossgrambo, this PR looks good to me.

[rossgrambo]

Most benchmarking was done in VS- but it doesn't have a great way to compare. BenchmarkDotNet does! Here's the result (4.0.0/4.9999.9999 includes these changes):

[rossgrambo]

Ran test again using .net8 (previous screenshot was .net6). This version is actually the most performant when using .net 8.