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

Bad LOH allocations when deregistering from IOptionsMonitor #112127

Open
Tragetaschen opened this issue Feb 4, 2025 · 11 comments
Open

Bad LOH allocations when deregistering from IOptionsMonitor #112127

Tragetaschen opened this issue Feb 4, 2025 · 11 comments
Labels
Milestone

Comments

@Tragetaschen
Copy link
Contributor

Description

We have an Orleans cluster and in the silo, each grain depends on global configuration. Since that can change during runtime, we are using IOptionsMonitor and when the grain is done, we dispose the handle we got from the OnChange call. That last dispose call is the lone source of some hefty LOH allocations. The grains come and go constantly and at a certain load on the silo (around 2500 grains), LOH allocations start to happen.

As far as I can tell, IOptionsMonitor.Dispose only deregisters from the internal multicast delegate and this deregistration is the main culprit: The multicast delegate allocates a new invocation list for each -=, so memory consumption becomes O(n) while it's amortized O(1) for += due to exponential growth.

This small benchmark shows the problem. The allocations grow with the number of already registered delegates.

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkRunner.Run<Benchmark>();

[SimpleJob(iterationCount: 5)]
[MemoryDiagnoser]
public class Benchmark
{
    private Action? _methods;

    [Params(8, 10, 12, 14)]
    public int Power { get; set; }

    [GlobalSetup]
    public void Setup()
    {
        _methods += Method;
        for (var i = 0; i < Power; ++i)
        {
            _methods += _methods;
        }
    }

    [Benchmark]
    public void AddRemove()
    {
        var m = new Action(Method);
        _methods += m;
        _methods -= m;
    }

    private void Method() { }
}
| Method    | Power | Mean       | Error      | StdDev     | Gen0     | Gen1     | Gen2     | Allocated |
|---------- |------ |-----------:|-----------:|-----------:|---------:|---------:|---------:|----------:|
| AddRemove | 8     |   1.512 us |  0.8605 us |  0.2235 us |   0.7629 |   0.0172 |        - |   6.23 KB |
| AddRemove | 10    |   5.376 us |  2.8169 us |  0.4359 us |   2.9602 |   0.1831 |        - |  24.23 KB |
| AddRemove | 12    |  18.840 us |  1.0445 us |  0.1616 us |  11.7493 |   1.4648 |        - |  96.23 KB |
| AddRemove | 14    | 135.817 us | 74.9703 us | 19.4695 us | 124.8779 | 124.8779 | 124.8779 | 384.28 KB |
@Tragetaschen Tragetaschen added the tenet-performance Performance related issue label Feb 4, 2025
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 4, 2025
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Feb 4, 2025
@huoyaoyuan huoyaoyuan added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Feb 4, 2025
@tarekgh
Copy link
Member

tarekgh commented Feb 4, 2025

@MihaZupan this issue is complaining about the perf of the delegate registration and deregistration. It is not related to the extension's options.

@MihaZupan
Copy link
Member

MihaZupan commented Feb 4, 2025

A multicast delegate is immutable. Adding/removing items will be slow as the collection grows.
Maybe there's room for improvement, but I wouldn't expect things to fundamentally change complexity-wise.

The issue is complaining that registering and unregistering from IOptionsMonitor many times is extremely slow once you have lots of callbacks.
If this is a scenario that extension options wants to improve, it should likely be using a different mechanism, e.g. a Dictionary (hence why I switched the areas).

@tarekgh
Copy link
Member

tarekgh commented Feb 4, 2025

Maybe there's room for improvement, but I wouldn't expect things to fundamentally change complexity-wise.

From the benchmark numbers, it looks to me the allocated memory is exponentially growing as you increase the registration/deregistration. Sure, we can optimize the Options part, but this is not going to help in the general case for users using such multicast delegates.

@Tragetaschen
Copy link
Contributor Author

Tragetaschen commented Feb 4, 2025

The benchmark is increasing the number of registered baseline delegates exponentially (2^Power). The memory growth for the individual deregistration is only linear in the number of registered delegates.

@Tragetaschen
Copy link
Contributor Author

Since the multicast delegate is immutable, there is an optimization along the += path where an existing invocation list is reused and shared between different immutable instances as long as there is room in the underlying array.

@tarekgh
Copy link
Member

tarekgh commented Feb 4, 2025

@Tragetaschen does your concern regarding IOptionsMonitor only?

@Tragetaschen
Copy link
Contributor Author

In our scenario, this turned up only along the OptionsMonitor.ChangeTrackerDisposable.Dispose call chain.

@tarekgh tarekgh added this to the Future milestone Feb 4, 2025
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Feb 4, 2025
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-extensions-options
See info in area-owners.md if you want to be subscribed.

@Tragetaschen
Copy link
Contributor Author

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Options;

BenchmarkRunner.Run<Benchmark>();

[SimpleJob(iterationCount: 5)]
[MemoryDiagnoser]
public class Benchmark
{
    private IOptionsMonitor<MyOptions> _optionsMonitor = null!;

    [Params(256, 1024, 4096, 8192)]
    public int BaseRegistrations { get; set; }

    [GlobalSetup]
    public void Setup()
    {
        var collection = new ServiceCollection();
        collection.AddOptions<MyOptions>();
        collection.Configure<MyOptions>(x => x.Value = 42);
        var services = collection.BuildServiceProvider();

        _optionsMonitor = services.GetRequiredService<IOptionsMonitor<MyOptions>>();

        for (var i = 0; i < BaseRegistrations; ++i)
        {
            // Registrations don't need to be removed
            _optionsMonitor.OnChange(new Action<MyOptions>(Method));
        }
    }

    [Benchmark]
    public void AddRemove()
    {
        var registration = _optionsMonitor.OnChange(new Action<MyOptions>(Method));
        registration?.Dispose();
    }

    private void Method(MyOptions _) { }

    private class MyOptions
    {
        public int Value { get; set; }
    }
}
| Method    | BaseRegistrations | Mean       | Error     | StdDev     | Gen0    | Gen1    | Gen2    | Allocated |
|---------- |------------------ |-----------:|----------:|-----------:|--------:|--------:|--------:|----------:|
| AddRemove | 256               |   1.603 us |  1.455 us |  0.3780 us |  0.7915 |  0.0191 |       - |   6.48 KB |
| AddRemove | 1024              |   5.316 us |  2.997 us |  0.7783 us |  2.9831 |  0.2670 |       - |  24.48 KB |
| AddRemove | 4096              |  20.829 us |  9.453 us |  2.4549 us | 11.7493 |  2.9297 |       - |  96.48 KB |
| AddRemove | 8192              | 121.139 us | 39.831 us | 10.3440 us | 41.6260 | 41.6260 | 41.6260 | 192.49 KB |

@jkotas
Copy link
Member

jkotas commented Feb 4, 2025

this is not going to help in the general case for users using such multicast delegates.

Multicast delegates are not optimized for this scenario by design. Optimizing multicast delegates for this scenario would be very difficult and it would hurt the common multicast delegate use cases with just a few subscribers that are not changing frequently.

@tarekgh
Copy link
Member

tarekgh commented Feb 4, 2025

I already tagged the issue back with Options area and will be scoped to IOptionsMonitor case.

@Tragetaschen Tragetaschen changed the title Bad LOH allocations when deregistering from a multicast delegate Bad LOH allocations when deregistering from IOptionsMonitor Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants