From c777c6a85aaa27bd11b7128415ff08e018bc166d Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Thu, 18 Jun 2020 01:29:26 +0200 Subject: [PATCH 1/7] Move `String.length` to the top of its module so that the `length` function is in scope --- src/fsharp/FSharp.Core/string.fs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/fsharp/FSharp.Core/string.fs b/src/fsharp/FSharp.Core/string.fs index df51fccd031..7e4b65f1a08 100644 --- a/src/fsharp/FSharp.Core/string.fs +++ b/src/fsharp/FSharp.Core/string.fs @@ -12,6 +12,9 @@ namespace Microsoft.FSharp.Core [] [] module String = + [] + let length (str:string) = if isNull str then 0 else str.Length + [] let concat sep (strings : seq) = String.Join(sep, strings) @@ -101,6 +104,3 @@ namespace Microsoft.FSharp.Core else let rec check i = (i < str.Length) && (predicate str.[i] || check (i+1)) check 0 - - [] - let length (str:string) = if isNull str then 0 else str.Length From 3a91ebcbdcebc3700b63da8cc8472e241d4a1b17 Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Thu, 18 Jun 2020 01:44:36 +0200 Subject: [PATCH 2/7] Using known length for mapping, and unrolling x2 to improve perf of String.mapi by 2.5x --- src/fsharp/FSharp.Core/string.fs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/fsharp/FSharp.Core/string.fs b/src/fsharp/FSharp.Core/string.fs index 7e4b65f1a08..b6997b8c02a 100644 --- a/src/fsharp/FSharp.Core/string.fs +++ b/src/fsharp/FSharp.Core/string.fs @@ -43,13 +43,24 @@ namespace Microsoft.FSharp.Core [] let mapi (mapping: int -> char -> char) (str:string) = - if String.IsNullOrEmpty str then + let len = length str + if len = 0 then String.Empty else - let res = StringBuilder str.Length - let f = OptimizedClosures.FSharpFunc<_,_,_>.Adapt(mapping) - str |> iteri (fun i c -> res.Append(f.Invoke(i, c)) |> ignore) - res.ToString() + let result = str.ToCharArray() + let f = OptimizedClosures.FSharpFunc<_,_,_>.Adapt mapping + + // x2 unrolled loop gives 10-20% boost, overall 2.5x SB perf + let mutable i = 0 + while i < len - len % 2 do + result.[i] <- f.Invoke(i, result.[i]) + result.[i + 1] <- f.Invoke(i, result.[i + 1]) + i <- i + 2 + + if i % 2 = 1 then + result.[i] <- f.Invoke(i, result.[i]) + + new String(result) [] let filter (predicate: char -> bool) (str:string) = From 3744f5301c08149f3e1ed5fc8bc159c124a72898 Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Thu, 18 Jun 2020 15:01:40 +0200 Subject: [PATCH 3/7] Fix two unapparent errors in the unroll loop, fix nit on parens --- src/fsharp/FSharp.Core/string.fs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/fsharp/FSharp.Core/string.fs b/src/fsharp/FSharp.Core/string.fs index b6997b8c02a..38f7da15e70 100644 --- a/src/fsharp/FSharp.Core/string.fs +++ b/src/fsharp/FSharp.Core/string.fs @@ -48,16 +48,17 @@ namespace Microsoft.FSharp.Core String.Empty else let result = str.ToCharArray() - let f = OptimizedClosures.FSharpFunc<_,_,_>.Adapt mapping + let f = OptimizedClosures.FSharpFunc<_,_,_>.Adapt(mapping) // x2 unrolled loop gives 10-20% boost, overall 2.5x SB perf let mutable i = 0 while i < len - len % 2 do result.[i] <- f.Invoke(i, result.[i]) - result.[i + 1] <- f.Invoke(i, result.[i + 1]) - i <- i + 2 + i <- i + 1 + result.[i] <- f.Invoke(i, result.[i]) + i <- i + 1 - if i % 2 = 1 then + if len % 2 = 1 then result.[i] <- f.Invoke(i, result.[i]) new String(result) From 4d15697056823658f3683302a885a12b3dba6107 Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Thu, 18 Jun 2020 15:02:19 +0200 Subject: [PATCH 4/7] Add String.mapi tests for all new unrolling corner cases, and some general ones --- .../StringModule.fs | 26 ++++++++++++++++--- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs index a72ab64e236..b764d3c1ac4 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs @@ -79,11 +79,29 @@ type StringModule() = [] member this.MapI() = - let e1 = String.mapi (fun i c -> char(int c + i)) "foo" - Assert.AreEqual("fpq", e1) + let e1 = String.mapi (fun _ c -> c) "12345" + Assert.AreEqual("12345", e1) - let e2 = String.mapi (fun i c -> c) null - Assert.AreEqual("", e2) + let e2 = String.mapi (fun _ c -> c + char 1) "1" + Assert.AreEqual("2", e2) + + let e3 = String.mapi (fun _ c -> c + char 1) "AB" + Assert.AreEqual("BC", e3) + + let e4 = String.mapi (fun i c -> char(int c + i)) "foo" + Assert.AreEqual("fpq", e4) + + let e5 = String.mapi (fun _ c -> c) null + Assert.AreEqual("", e5) + + let e6 = String.mapi (fun _ c -> c) String.Empty + Assert.AreEqual("", e6) + + let e7 = String.mapi (fun _ _ -> failwith "should not fail") null + Assert.AreEqual("", e7) + + let e8 = String.mapi (fun i _ -> if i = 1 then failwith "should not fail" else char i) "X" + Assert.AreEqual("\u0000", e8) [] member this.Filter() = From 252253814bd3cfb457143e3bc16e665dac56d50e Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Thu, 18 Jun 2020 17:14:59 +0200 Subject: [PATCH 5/7] Add side-effect test, this would fail if sequentiality of String.mapi is broken --- .../Microsoft.FSharp.Collections/StringModule.fs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs index b764d3c1ac4..5e3eb56e39e 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs @@ -103,6 +103,12 @@ type StringModule() = let e8 = String.mapi (fun i _ -> if i = 1 then failwith "should not fail" else char i) "X" Assert.AreEqual("\u0000", e8) + // side-effect and "order of operation" test + let mutable x = 0 + let e9 = String.mapi (fun i c -> x <- x + i; c + char x) "abcde" + Assert.AreEqual(x, 10) + Assert.AreEqual(e9, "acfjo") + [] member this.Filter() = let e1 = String.filter (fun c -> true) "foo" From 82a2040879a468fbc8d4dd03700a3dffed7b68c5 Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Fri, 19 Jun 2020 18:42:41 +0200 Subject: [PATCH 6/7] Remove "foo" from tests --- .../FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs index 5e3eb56e39e..c7ec21c5721 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs @@ -88,8 +88,8 @@ type StringModule() = let e3 = String.mapi (fun _ c -> c + char 1) "AB" Assert.AreEqual("BC", e3) - let e4 = String.mapi (fun i c -> char(int c + i)) "foo" - Assert.AreEqual("fpq", e4) + let e4 = String.mapi (fun i c -> char(int c + i)) "hello" + Assert.AreEqual("hfnos", e4) let e5 = String.mapi (fun _ c -> c) null Assert.AreEqual("", e5) From 3936ad035138c2a90a27061b415dacc1d6dd95f1 Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Fri, 19 Jun 2020 18:46:16 +0200 Subject: [PATCH 7/7] Remove unrolling of loop in String.mapi --- src/fsharp/FSharp.Core/string.fs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/fsharp/FSharp.Core/string.fs b/src/fsharp/FSharp.Core/string.fs index 38f7da15e70..cdd19f9a385 100644 --- a/src/fsharp/FSharp.Core/string.fs +++ b/src/fsharp/FSharp.Core/string.fs @@ -50,16 +50,10 @@ namespace Microsoft.FSharp.Core let result = str.ToCharArray() let f = OptimizedClosures.FSharpFunc<_,_,_>.Adapt(mapping) - // x2 unrolled loop gives 10-20% boost, overall 2.5x SB perf let mutable i = 0 - while i < len - len % 2 do + while i < len do result.[i] <- f.Invoke(i, result.[i]) i <- i + 1 - result.[i] <- f.Invoke(i, result.[i]) - i <- i + 1 - - if len % 2 = 1 then - result.[i] <- f.Invoke(i, result.[i]) new String(result)