String map performance improvement

[abelbraaksma]

As in the title: Improve performance of String.map by 2 - 2.5x and reduce mem and GC pressure by 10%. For details of measurements, see #9390 (comment)

I decided not to go for special-casing small values or loop-unrolling as for this specific case, the benefits don't outweigh the costs of the added complexity.

The chosen approach is the array-based approach.

[Rodrigo-Andrade]

I guess String.Create api is not available?

[abelbraaksma]

@Rodrigo-Andrade Indeed, see the link, I investigated that, and showed timings (which are better), though I knew it cannot be used because it creates a dependency for framework projects.

Though this is still a reasonable improvement, I think.

[forki]

Cool stuff

[vzarytovskii]

It seems, that the changeset in this PR is identical to #9469.

[KevinRansom]

@abelbraaksma,
Your fix is neat, and I like it a lot, however, in the BCL in order to reduce stringbuilder allocations, we implemented a StringBuilder cache sourcecode here:

https://github.com/dotnet/runtime/blob/7af8486e6aa8d73612e36841f87aa42d7f253ea0/src/libraries/Common/src/System/Text/StringBuilderCache.cs

It was used in a bunch of places in the BCL to reduce the creation of string builders for small strings. It is used extensively in BCL .ToString() implementations funnily enough. I have often wondered whether it would be of benefit in FSharp.Core.

Here is an example of it in use: https://github.com/dotnet/runtime/blob/8f8cfd446db5227d2685b7c0fe63084c776019ff/src/libraries/System.Net.Http/src/System/Net/Http/Headers/TransferCodingHeaderValue.cs#L123

Do you think it would be worthwhile trying it out, to see if it is beneficial, because we could probably use it to improve allocations of most stringbuilders in FSharp.Core.

Effectively for small strings the StringBuilder cache would eliminate keep reallocating a string builder. At the cost of 1 string builder per thread.

[abelbraaksma]

@KevinRansom, several methods that were used originally, like String::Join, internally call System.Text.StringBuilder System.Text.StringBuilderCache::Acquire(int32) and friends. Meaning, the cache was indeed used, but was not faster.

I noticed with many timings, when the expected size is known beforehand, it is just much, much faster in almost all cases to not use either the StringBuilder or the StringBuilderCache.

This is in part because the constructor of String that takes a char[] is essentially an intrinsic and internally calls a highly optimized loop. It also appears to have to do with StringBuilder.Append adding a lot of overhead compared to stelem in IL (essentially, in the end they do the same, but a single stelem is just over twice as fast compared to calling Append, which needs to track its state).

In my own code I usually choose StringBuilder when target size cannot be determined statically. Otherwise, I use Span and in very hot paths I use fixed (neither of which are in the upcoming PRs). The advantages of the latter are: half the allocations and tighter loops. If you create a new string with StringBuilder or char[], the allocations are equal to 4x the result string (for String.map) (plus a little overhead, sometimes a lot, for SB), if you do it with Span or fixed, it goes down to the more reasonable 2x (one for the given string, one for the newly created string).

Do you think it would be worthwhile trying it out, to see if it is beneficial, because we could probably use it to improve allocations of most stringbuilders in FSharp.Core.

I've been thinking about that a bit. But the same rules in general apply: the cached builder is already used in many cases through other BCL methods and it still adds a lot of overhead, esp for small concats, compared to either char[] or calling the first four overloads of String.Concat. We'll have to investigate specific cases, like I did in the relevant issue for String.xxx, whether one is faster over the other.

For uses in FCS that are string-intensive, we might get a lot of benefit by using Span and friends, or even fixed in tight spots where that makes sense. This will work where SB is not a good candidate (if you know, or can simply calculate, the target size).

[abelbraaksma]

Sourcelink errors, not sure what causes this: https://github.com/dotnet/fsharp/pull/9470/checks?check_run_id=780410684

[abelbraaksma]

It seems, that the changeset in this PR is identical to #9469.

@vzarytovskii, my apologies. This is meanwhile fixed. I accidentally cross-merged between branches.

[KevinRansom]

@abelbraaksma , yes when the size is known ahead of time a single allocation is waaaay better than stringbuilder. However, if the cached sb is already bigger than the string to be built then there should be no allocation at all.

I've been thinking about that a bit. But the same rules in general apply: the cached builder is
already used in many cases through other BCL methods and it still adds a lot of overhead, esp for small concats, compared to either char[] or calling the first four overloads of String.Concat.

This surprises me, because the benefit of the cache was supposed to be for small stringbuilds. If the required string is bigger than the cache limit then a new sb is used, if the sb grows above the limit a new sb is used.
Now that I have been reminded of it perhaps I can do some testing to see if it can help us generally. Sigh !!!! priorities.

[abelbraaksma]

then there should be no allocation at all.

@KevinRansom That was my take as well, and I turned out to be wrong. There's always the allocation of the final string, which copies the relevant buffer to the immutable string. Real advancement can only be made if we skip the extra copy step (fixed, span, or use FastAllocateString directly).

In some cases the buffering is the fastest method, sometimes it isn't. The overhead of the buffering never wins from String.Concat: string * string -> string, which is also start C# uses when it can. In the referenced issue you can find timings for each of these methods.

There's certainly no "one size fits all", and after learning a lot about SB, I'm still surprised it's often so much slower. My take is that it has to do with the method call overhead, as it's called for each 16 bit char. The differences are much less (but still present) with String.init and String.collect.

Perhaps we should continue this in the main issue? As it applies to almost all functions. #9390.

[KevinRansom]

@abelbraaksma the returned string allocation, is a constant requirement. However, the sb cache should only require that one allocation for strings under 360 chars. Whereas this PR, (and it's definitely a big improvement over the existing version, requires 2 allocations), the initial buffer on line 27 and the returned string.

I would really like to understand whether there is any benefit to the SB cache, because we could probably use that same technique where-ever we use an SB in fsharp.core.

[abelbraaksma]

requires 2 allocations), the initial buffer on line 27 and the returned string.

I thought the same, but the bug mystery is, it requires 3. This followed from BDN analysis and was constant: with the initial string, it required 4x the mem size of the string.

Same is true for SB, but maybe I missed something.

I discussed this in Slack with @baronfel, but we couldn't explain the extra allocation.

With fixed it was only 2x mem (1 for input, one for output). The extra alloc is a mystery that I've yet to solve.

there is any benefit to the SB cache, because we could probably use that same technique where-ever we use an SB in fsharp.core.

@KevinRansom, I linked an example in the main issue, the benefit shows there clearly when used with enumerables.

[KevinRansom]

I will take a look, there could be a bug in SB I suppose, or even the cache. These mysteries are what make our job fun and miserable at the same time. I have your code in the copy buffer, as soon as I finish the email I'm on, I'm gonna look. BTW @cartermp is a l33t nerdsniper but it seems as if you are approaching his nerd-snipping skillz.

:-)

[NinoFloris]

@abelbraaksma do you have the bdn benchmark somewhere?

[abelbraaksma]

@KevinRansom, I'm also on Slack, if you prefer an in depth discussion of findings and possibilities, I've investigated a lot in string optimizations in my own parser/compiler, we can always set up a brainstorm session or call :)

[abelbraaksma]

@NinoFloris, did you check the linked issue? All relevant timings are there.

[KevinRansom]

That would be good, however, I am at home, with two noisy kids running about. So I expect to be a bit distracted for a while.

[NinoFloris]

I was mostly interested in

I thought the same, but the bug mystery is, it requires 3.

Not sure which of the functions shows that phenomenon

[abelbraaksma]

@NinoFloris, it's this function (it applies to more functions there, but this was the main focus):

let fasterMap mapper str =
    if String.IsNullOrEmpty str then String.Empty
    else
        let result = str.ToCharArray()
        let mutable i = 0
        for c in result do
            result.[i] <- mapper c
            i <- i + 1
        String result

To compare, the following gives 2x input size as mem use, as opposed 4x input size for the above function and the current function (by which I mean: String.map):

    let fixedStrCopySrcIdx2 (mapper: char -> char) (str: string) =
        let len = String.length str
        let result = String.Copy str
        use shadow = fixed result
        let mutable i = 0
        while i < len - len % 2 do
            NativePtr.set shadow i (mapper str.[i])
            NativePtr.set shadow (i + 1) (mapper str.[i + 1])
            i <- i + 2

        while i < len do
            NativePtr.set shadow i (mapper str.[i])
            i <- i + 1

        result

The timings of this function can be found in the tables of this comment: #9390 (comment).

The BDN benchmark function looks like this, where Data.get simply gets data based on the size of this.Value and Data.getMapper for clean mem analysis only gives the id function:

[<SimpleJob(RuntimeMoniker.NetCoreApp31)>]
[<MemoryDiagnoser>]
[<CategoriesColumn; RankColumn>]
[<DisassemblyDiagnoser>]
type BenchCandidates() =

    [<DefaultValue>]
    [<Params(0, 1, 2, 3, 4, 5, 6, 7, 8, 10, 31, 100, 256, 1024, 8192)>]
    val mutable Value: int

    [<DefaultValue>]
    [<ParamsAllValues>]
    val mutable Mapper: Data.Mapper

    [<Benchmark(Baseline=true); BenchmarkCategory "Candidate">]
    member this.current() =
        let mapper = Data.getMapper this.Mapper
        Data.get this.Value
        |> String.map mapper
        |> ignore

    [<Benchmark; BenchmarkCategory "Candidate">]
    member this.fasterMap() =
        let mapper = Data.getMapper this.Mapper
        Data.get this.Value
        |> StringMap.fasterMap mapper
        |> ignore

The module Data specifically used let bindings on module level to prevent extra allocations on creation and other stuff that gets in the way of clean timings:

module Data =
    let get value =
        match value with
        | 0 -> String.Empty
        | 1 -> "A"
        | 2 -> "Ab"
        // etc
        | 1024 -> large1024  // `global let bindings so they're loaded only once during cctor

[KevinRansom]

@abelbraaksma well you totally nerd-sniped me.

I reimplemented StringBuilderCache and reworked the func to use it and I get these results, and they are mixed.
mapOldBenchMark is the existing code.
mapNewBenchMark is the alternative in this PR
mapCachedSBBenchMark is using the cached string builder.
all code below ...

The string being mapped is 30 characters : ''''C:\kevinransom\interactive\src''''

|               Method |        Mean |     Error |    StdDev |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------------- |------------:|----------:|----------:|-------:|------:|------:|----------:|
|      mapOldB...k | 178.6543 ns | 1.7177 ns | 1.4344 ns | 0.0446 |     - |     - |     280 B |
| mapCachedSBB...k | 130.4789 ns | 2.3877 ns | 1.9939 ns | 0.0176 |     - |     - |     112 B |
|      mapNewB...k |  72.3534 ns | 1.4475 ns | 1.3540 ns | 0.0318 |     - |     - |     200 B |

It should be noted that perf in this PR is unbeatable, which was to be expected. The character copying is just array element to array element ... that's really ... really nice.

The allocations however using the cached String Builder are slightly more than twice the improvements in this PR.

We could combine this PR along with a CharBuffer class to get the memory improvements and the great perf. If we were to go with that approach, we could use it all over the bcl when doing this kind of char copying code. What do you think?

module MyModule

open System
open System.Runtime.InteropServices
open System.Text
open BenchmarkDotNet.Attributes
open BenchmarkDotNet.Running

/// <summary>Provide a cached reusable instance of stringbuilder per thread.</summary>
type StringBuilderCache private () =

    // The value 360 was chosen in discussion with performance experts as a compromise between using
    // as litle memory per thread as possible and still covering a large part of short-lived
    // StringBuilder creations on the startup path of VS designers.
    static let maxBuilderSize = 360
    static let defaultCapacity = 16    // == StringBuilder.DefaultCapacity

    // WARNING: We allow diagnostic tools to directly inspect this member (t_cachedInstance).
    // See https://github.com/dotnet/corert/blob/master/Documentation/design-docs/diagnostics/diagnostics-tools-contract.md for more details.
    // Please do not change the type, the name, or the semantic usage of this member without understanding the implication for tools.
    // Get in touch with the diagnostics team if you have questions.
    [<ThreadStatic>] [<DefaultValue>] 
    static val mutable private _cachedInstance:StringBuilder

    /// <summary>Get a StringBuilder for the specified capacity.</summary>
    /// <remarks>If a StringBuilder of an appropriate size is cached, it will be returned and the cache emptied.</remarks>
    static member Acquire ([<Optional; DefaultParameterValue(16)>] capacity:int) : StringBuilder =
        if capacity <= maxBuilderSize then
            let cached =
                match StringBuilderCache._cachedInstance with
                | null  ->  StringBuilder(capacity)
                | sb when capacity > sb.Capacity -> StringBuilder(capacity)
                | sb ->
                    StringBuilderCache._cachedInstance <- Unchecked.defaultof<StringBuilder>
                    sb
            cached.Clear() |> ignore
            cached
        else
            StringBuilder(capacity)

    static member Release(sb: StringBuilder) =
        if sb.Capacity <= maxBuilderSize then
            StringBuilderCache._cachedInstance <- sb

    /// <summary>ToString() the stringbuilder, Release it to the cache, and return the resulting string.</summary>
    static member GetStringAndRelease(sb:StringBuilder): string =
        let result = sb.ToString()
        StringBuilderCache.Release(sb)
        result;

[<MemoryDiagnoser>]
type BenchMarks () =
    let myString = @"C:\kevinransom\interactive\src"

    let iter (action : (char -> unit)) (str:string) =
        if not (String.IsNullOrEmpty str) then
            for i = 0 to str.Length - 1 do
                action str.[i] 

    let noWork (mapping: char -> char) (str:string) =
        if String.IsNullOrEmpty str then
            String.Empty
        else
            str

    let mapOld (mapping: char -> char) (str:string) =
        if String.IsNullOrEmpty str then
            String.Empty
        else
            let res = StringBuilder str.Length
            str |> iter (fun c -> res.Append(mapping c) |> ignore)
            res.ToString()

    let mapCachedSB (mapping: char -> char) (str:string) =
        if String.IsNullOrEmpty str then
            String.Empty
        else
            let result = StringBuilderCache.Acquire(str.Length)
            try
                result.Clear() |> ignore
                for c in str do
                    result.Append (mapping c) |> ignore
                result.ToString()

            finally
                StringBuilderCache.Release(result)
                
    let mapNew (mapping: char -> char) (str:string) =
        if String.IsNullOrEmpty str then
            String.Empty
        else
            let result = str.ToCharArray()
            let mutable i = 0
            for c in result do
                result.[i] <- mapping c
                i <- i + 1

            new String(result)

    [<Benchmark>]
    member _.noWorkBenchMark (): string = noWork (fun c -> '@') myString

    [<Benchmark>]
    member _.mapOldBenchMark (): string =  mapOld (fun c -> '@') myString

    [<Benchmark>]
    member _.mapCachedSBBenchMark (): string = mapCachedSB (fun c -> '@') myString

    [<Benchmark>]
    member _.mapNewBenchMark(): string = mapNew (fun c -> '@') myString


[<EntryPoint>]
let main argv =
    let bm = new BenchMarks()

    printfn "noWorkBenchMark : %s" (bm.noWorkBenchMark())
    printfn "mapOldBenchMark : %s" (bm.mapOldBenchMark())
    printfn "mapSingleSBBenchMark : %s" (bm.mapCachedSBBenchMark())
    printfn "mapNewBenchMark : %s" (bm.mapNewBenchMark())
    let _summary = BenchmarkRunner.Run<BenchMarks>()
    0 // return an integer exit code

[KevinRansom]

@abelbraaksma ..

Yes, it can be sped up, array has direct access, string uses a pointer dereference for each char access to skip the first 8 bytes (which contains the length), but the speed up depends on other factors as well.

How would we write the string copy to be faster?

[KevinRansom]

One other thing, we have tried to minimize the dependency that FSharp.Core takes so taking a dependency on System.Buffer is probably not something we will do. It is in netstandard2.1 and would require us to stop targeting desktop for FSharp.Core apps. However, the code for the cache isn't a lot of code and the grabbed memory is small. The benefit accrues to smaller strings, but there are many more of those than stonking great large strings. If I can speed up the iteration I think I would prefer to take the cached version, because lowering allocations is pretty helpful
. What do you think?

[baronfel]

@KevinRansom the version of System.Buffers I see on Nuget says it supports net45 and net461, surely that qualifies? https://www.nuget.org/packages/System.Buffers/

[KevinRansom]

@baronfel , in an ideal world you would be correct, however, everyone that ships an App dependent on FSharp.Core which is almost everybody who builds a desktop app using the fsharp compiler would also have to ship System.Buffers.dll. I'm absolutely certain that loads of people would get that wrong. We messed up when took a dependency on it in VS. Today FSharp.Core only has a dependency on things that can be retrieved from the desktop framework, with the exception of ValueTuple, which it needs for reflection helpers. And even then everything else works if ValueTuple is not found.

Kevin

[abelbraaksma]

@KevinRansom, there's a discussion about ImmutableArray to be implemented through a dependency on System.Collections.Immutable.dll. If that is approved, this will give us System.Memory and System.Buffers as well, which inclused Span. But that's only if that dependency is going to happen.

I've created a discussion topic here: #9501, as we have veered quite far from just this PR ;).

[KevinRansom]

@abelbraaksma , @cartermp says you have an implemention using fixed, can we take a look at that again. That seems like the lowest friction, and likely best performing approach to take.

[abelbraaksma]

@KevinRansom, in the light of this issue? Or as a more general approach, because I'd like to move that to #9501 (I'll update there with some answers to your questions).

Indeed, I did make it for String.map, I posted it already in this PR as a comment to @NinoFloris, he asked about it too, see: #9470 (comment)

The timings are published here: #9470 (comment). If you check, for instance, the 100 chars section, you can see that this PR (fasterMap) takes 311.8ns, 472 bytes, while using native pointers and fixed, we get 226ns or 234ns (depending on method) with each 248 bytes.

The fixed version is the winner overall, as it both fastest and uses half the memory of any other method (except Span-based). The reason is simple: it only requires to create the target string once and overwrites it (just like BCL does internally when you call String: char[] -> string), while all other approaches require a working copy and a copy for the final string (plus the mysterious 3rd copy).

The simplest version, without unrolling, looks as follows (the unrolled version is in the comment above):

let mapWithFixed(mapper: char -> char) (str: string) =

    let len = String.length str
    let result = String.Copy str

    use shadow = fixed result
    let mutable i = 0
    while i < len do
        NativePtr.set shadow i (mapper str.[i])
        i <- i + 1

    result

Note that at the bottom of that comment all versions I tried, including the ones using String.Create and ReadOnlySpan + Span can be found.

[KevinRansom]

You’ve nerd snipped both me and Will now. We are having such a lot of fun in the convo will be fine.

[abelbraaksma]

You’ve nerd snipped both me and Will now.

I really have to put that up on my chimney (if I still had one), you're honestly the first ever who I've seen using the phrase "nerd snipped", but it certainly makes me smile :).

Btw, who's Will? Smith?

[KevinRansom]

@abelbraaksma can you ping me using my work email address pls.

[KevinRansom]

Okay I logged into fsharp.org general as codecutter.

[KevinRansom]

Will is indeed Mr Smith.

[KevinRansom]

This does a good job on reducing allocations, and is faster too.

[abelbraaksma]

edit, I've moved my comment here: #9501 (comment)

[abelbraaksma]

Last change 83ac13d is only in tests, following up on a comment from @KevinRansom to get rid of "foo" in tests, just kind find the original comment. Anyway, nothing functionally changed, all's still green.