Pooling StringBuilder instances

[alexeyzimarev]

OpenTelemetry for .NET extensively uses StringBuilder that is continuously instantiated and, therefore, allocated and then loads the GC.

The object pooling ASP.NET Core extensions allow pooling object instances, in particular, StringBuilder even has a default policy. It is quite easy to use but it also introduces a dependency. Would it be a good idea to implement a simplified string builder pool inside the opentelemetry-net project and use it?

I worry especially about exporters that will be extensively used.

[cijothomas]

Will the dependency be required only for exporter projects? or OT SDK itself?

[eddynaka]

@cijothomas , i think @alexeyzimarev was talking about ObjectPool https://docs.microsoft.com/en-us/aspnet/core/performance/objectpool?view=aspnetcore-3.1, which we would need to install https://www.nuget.org/packages/Microsoft.Extensions.ObjectPool.

Basically, we would need to inject:

services.TryAddSingleton<ObjectPool<StringBuilder>>(serviceProvider =>
        {
            var provider = serviceProvider.GetRequiredService<ObjectPoolProvider>();
            var policy = new StringBuilderPooledObjectPolicy();
            return provider.Create(policy);
        });

and, then, use it:

public async Task InvokeAsync(ObjectPool<StringBuilder> builderPool)
{          
    // Request a StringBuilder from the pool.
    var stringBuilder = builderPool.Get();
    // some code
} 

[alexeyzimarev]

That's right. I think it can significantly decrease allocations but it introduces a dependency. Maybe it will be built-in in .NET 5, who knows.

[cijothomas]

Thanks for clarifying. @alexeyzimarev could you point couple of places where we use the string-builder and are candidates for this? We need to evaluate if using ObjectPool, or own implementation of similar would be justified.

[cijothomas]

Not sure if this is still relevant after all recent perf improvements in the exporters. Will check if we need this.

[mic-max]

These are all 13 places we call the StringBuilder constructor

The 6 uses we care about

• PrometheusMetricBuilder.cs function GetSafeName()
• HttpInListener.cs function GetUri()
• TraceContextPropagator.cs function TryExtractTracestate()
• BaggagePropagator.cs function Inject()
• B3Propagator.cs function Inject()
• TraceStateUtilsNew.cs function GetString()

I'm not familiar with the code base yet so don't know the expected amount of times each of these functions gets called per program execution.
Example Benchmark Link

---

The 7 uses we don't care about

• ConsoleMetricExporter.cs : Is not intended for production use
• MyExporter.cs : Is part of some documentation
• ZipkinExporterTests.cs : A test

[martinjt]

Do we still need to consider doing this? I know there's been lots of work on the allocations for the hotpath.

From what I can see, the only places we're using it now are in the SDK instantiation, the Prometheus listener setup and the console exporters. Since we don't consider the Console exporters to be the right way forward (in favour of OTLP), I'm not sure we'd keep this issue open for those.

The place where it would be potentially a thing is the propagators, however, since they're always fixed string length, I'm not sure pooling would be the answer there.

I'm inclined to close this.

@cijothomas @mic-max what are your thoughts? I don't want to keep this open for the sake of it if we're not planning on doing anything with it anytime soon.

[stevejgordon]

@martinjt I still StringBuilder used in the propagators. I've addressed one of these in a new PR. If that approach looks good, I can take a pass over the others.