dotnet icon indicating copy to clipboard operation
dotnet copied to clipboard

Add an overload to StringPool.GetOrAdd method which accepts an interpolated string

Open sonnemaf opened this issue 2 years ago • 6 comments

Overview

It would be really nice if there is an overload for the StringPool.GetOrAdd method which accepts a custom InterpolatedStringHandler. This InterpolatedStringHandler would create a ReadOnlySpanand use this to get the string from the pool without any string allocation (if available in the pool).

API breakdown

namespace CommunityToolkit.HighPerformance.Buffers;

class StringPool {

    public string GetOrAdd(CustomInterpolatedStringHandler value);
}

Usage example

using CommunityToolkit.HighPerformance.Buffers;

for (int i = 0; i < 100; i++) {
	var array = Foo();
	// some code using the array;
}

static Employee[] Foo() {
    var result = new Employee[100];
	for (int i = 0; i < result.Length; i++) {
		string name = StringPool.Shared.GetOrAdd($"Jim {i}"); // this would use the new overload

		result[i] = new Employee(name, i);
	}
	return result;
}

record class Employee(string Name, decimal Salary);

Breaking change?

No

Alternatives

Additional context

No response

Help us help you

Yes, but only if others can assist

sonnemaf avatar Sep 01 '23 15:09 sonnemaf

I found a solution which uses the new UnsafeAccessor attribute of .NET 8. The GetOrAddInterpolated() extension method on StringPool uses the ToSpanAndClear() method which call the UnsafeAccessor methods Text and Clear().

using CommunityToolkit.HighPerformance.Buffers;
using System.Runtime.CompilerServices;

Console.WriteLine(StringPool.Shared.GetOrAddInterpolated($"Hello, World! {args.Length}"));

for (int i = 0; i < 100_000; i++) {
    string s = StringPool.Shared.GetOrAddInterpolated($"Hello, World! {args.Length}");
}

static class Extensions {

    public static string GetOrAddInterpolated(this StringPool pool, DefaultInterpolatedStringHandler interpolatedStringHandler) {
        return pool.GetOrAdd(interpolatedStringHandler.ToSpanAndClear());
    }

    private static ReadOnlySpan<char> ToSpanAndClear(this ref DefaultInterpolatedStringHandler interpolatedStringHandler) {
        var span = interpolatedStringHandler.TextProperty();
        interpolatedStringHandler.ClearMethod();
        return span;
    }

    [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "get_Text")]
    private static extern ReadOnlySpan<char> TextProperty(this ref DefaultInterpolatedStringHandler @this);

    [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "Clear")]
    private static extern void ClearMethod(this ref DefaultInterpolatedStringHandler @this);

}

sonnemaf avatar Sep 16 '23 08:09 sonnemaf

You should probably use the span to get a string from the pool before clearing the builder, as an array could be holding on to the span, and be returned to the pool at Clear().

var span = interpolatedStringHandler.TextProperty();
var result = pool.GetOrAdd(span);
interpolatedStringHandler.ClearMethod();
return result;

ovska avatar Sep 23 '23 13:09 ovska

Yeah that ToSpanAndClear() is not safe. If you clear the builder, the array is returned to the pool, so another thread might concurrently rent the same array and write random data into it as you're still reading from that span.

Sergio0694 avatar Sep 23 '23 13:09 Sergio0694

This shows again how difficult it is to write thread-safe code. Thank you both for warning me.

I rewrote the solution to this:

using CommunityToolkit.HighPerformance.Buffers;
using System.Runtime.CompilerServices;

Console.WriteLine(StringPool.Shared.GetOrAddInterpolated($"Hello, World! {args.Length}"));

for (int i = 0; i < 100_000; i++) {
    string s = StringPool.Shared.GetOrAddInterpolated($"Hello, World! {args.Length}");
}

static class Extensions {

    public static string GetOrAddInterpolated(this StringPool pool, DefaultInterpolatedStringHandler interpolatedStringHandler) {
        try {
            var span = interpolatedStringHandler.GetText();
            return pool.GetOrAdd(span);
        } finally {
            interpolatedStringHandler.Clear();
        }
    }

    [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "get_Text")]
    private static extern ReadOnlySpan<char> GetText(this ref DefaultInterpolatedStringHandler @this);

    [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "Clear")]
    private static extern void Clear(this ref DefaultInterpolatedStringHandler @this);
}

sonnemaf avatar Sep 24 '23 10:09 sonnemaf

@sonnemaf Is your proposal accepted by the team for the next release of this toolkit? Or how are things being decided?

CodingMadness avatar Oct 18 '23 00:10 CodingMadness

I don't think it can be added to the Toolkit. The Toolkit is compiled into a .NET Standard library. I used .NET8 specific code (UnsafeAccessor) which does not work in .NET Standard. I'm afraid there is no solution for this. You will have to add this Extensions class to your project.

Maybe we can add a sourcegenerated solution to the project simular to what PolySharp is doing.

@Sergio0694 What do you think?

sonnemaf avatar Oct 24 '23 16:10 sonnemaf