CommunityToolkit / dotnet

.NET Community Toolkit is a collection of helpers and APIs that work for all .NET developers and are agnostic of any specific UI platform. The toolkit is maintained and published by Microsoft, and part of the .NET Foundation.
https://docs.microsoft.com/dotnet/communitytoolkit/?WT.mc_id=dotnet-0000-bramin
Other
2.99k stars 294 forks source link

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

Open sonnemaf opened 1 year ago

sonnemaf commented 1 year ago

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 commented 1 year ago

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

}
ovska commented 1 year ago

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;
Sergio0694 commented 1 year ago

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.

sonnemaf commented 1 year ago

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);
}
CodingMadness commented 11 months ago

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

sonnemaf commented 11 months ago

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?