Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't Clone Context in UnleashContext.ApplyStaticFields #193

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

nathanmascitelli
Copy link
Contributor

Description

When calling IUnleash.IsEnabled we end up cloning the UnleashContext. This cloning is not needed and expensive since the entire Properties dictionary needs to be cloned. This case removes the cloning.

Fixes #186

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Using the following benchmark (note IsEnabled2 calls the code proposed in this PR):

[MemoryDiagnoser]
public class Benchmark
{
    readonly UnleashContext context = new();
    readonly DefaultUnleash client = new(new UnleashSettings());

    public Benchmark()
    {
        // Remove this loop to test empty properties
        for (int i = 0; i < 10; i++)
        {
            context.Properties.Add(i.ToString(), i.ToString());
        }
    }

    [Benchmark(Baseline = true)]
    public bool Current() => client.IsEnabled("Moo", context);

    [Benchmark]
    public bool NoClone() => client.IsEnabled2("Moo", context);
}

The following results were obtained:

Empty Properties

Method Mean Error StdDev Ratio Gen0 Allocated Alloc Ratio
Current 179.3 ns 1.80 ns 1.69 ns 1.00 0.0408 512 B 1.00
NoClone 145.8 ns 1.06 ns 0.94 ns 0.81 0.0274 344 B 0.67

10 Elements in Properties

Method Mean Error StdDev Ratio Gen0 Allocated Alloc Ratio
Current 262.7 ns 2.64 ns 2.34 ns 1.00 0.0691 872 B 1.00
NoClone 144.8 ns 2.65 ns 3.45 ns 0.56 0.0274 344 B 0.39

@daveleek daveleek self-requested a review December 20, 2023 12:48
@daveleek daveleek self-assigned this Dec 20, 2023
Copy link
Collaborator

@daveleek daveleek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, thank you @nathanmascitelli!
I'll merge this and give it a run through when setting up the next release!

@daveleek daveleek merged commit 4838030 into Unleash:main Dec 20, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does the Context Need to be Cloned?
2 participants