search

microsoft / FeatureManagement-Dotnet

Microsoft.FeatureManagement provides standardized APIs for enabling feature flags within applications. Utilize this library to secure a consistent experience when developing applications that use patterns such as beta access, rollout, dark deployments, and more.
MIT License
1.06k stars 115 forks source link

Allocation cleanup #508

Closed rossgrambo closed 3 weeks ago

rossgrambo commented 1 month ago

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-

Visible Changes

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

rossgrambo commented 4 weeks ago

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

zhiyuanliang-ms commented 4 weeks ago

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

rossgrambo commented 4 weeks ago

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 commented 3 weeks ago

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

rossgrambo commented 3 weeks ago

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

image

rossgrambo commented 3 weeks ago

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