String map performance improvement (#9470)

* Simplify and improve perf of String.length

* Improve performance of String.map

* Revert "Simplify and improve perf of String.length"

* Resolves #9470 (comment)

* Lingering space

* Change `String` to use `new` to clarify use of ctor

* Add some better tests for String.map, add side-effect test

* Add tests to ensure the mapping function is called a deterministically amount of times

* Fix typo

* Remove "foo" from String.map tests

Author: abelbraaksma

src/fsharp/FSharp.Core/string.fs

@@ -42,9 +42,13 @@ namespace Microsoft.FSharp.Core
             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 result = str.ToCharArray()
+                let mutable i = 0
+                for c in result do
+                    result.[i] <- mapping c
+                    i <- i + 1
+
+                new String(result)
 
         [<CompiledName("MapIndexed")>]
         let mapi (mapping: int -> char -> char) (str:string) =

tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs

@@ -71,11 +71,35 @@ type StringModule() =
 
     [<Test>]
     member this.Map() =
-        let e1 = String.map (fun c -> c) "foo"
-        Assert.AreEqual("foo", e1)
+        let e1 = String.map id "xyz"
+        Assert.AreEqual("xyz", e1)
 
-        let e2 = String.map (fun c -> c) null 
-        Assert.AreEqual("", e2)
+        let e2 = String.map (fun c -> c + char 1) "abcde"
+        Assert.AreEqual("bcdef", e2)
+
+        let e3 = String.map (fun c -> c) null 
+        Assert.AreEqual("", e3)
+
+        let e4 = String.map (fun c -> c) String.Empty 
+        Assert.AreEqual("", e4)
+
+        let e5 = String.map (fun _ -> 'B') "A"
+        Assert.AreEqual("B", e5)
+
+        let e6 = String.map (fun _ -> failwith "should not raise") null
+        Assert.AreEqual("", e6)
+
+        // this tests makes sure mapping function is not called too many times
+        let mutable x = 0
+        let e7 = String.map (fun _ -> if x > 2 then failwith "should not raise" else x <- x + 1; 'x') "abc"
+        Assert.AreEqual(x, 3)
+        Assert.AreEqual(e7, "xxx")
+
+        // side-effect and "order of operation" test
+        let mutable x = 0
+        let e8 = String.map (fun c -> x <- x + 1; c + char x) "abcde"
+        Assert.AreEqual(x, 5)
+        Assert.AreEqual(e8, "bdfhj")
 
     [<Test>]
     member this.MapI() =