diff --git a/docs/release-notes/.FSharp.Compiler.Service/10.0.200.md b/docs/release-notes/.FSharp.Compiler.Service/10.0.200.md index 8b46840d37c..c49c3bd34a4 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/10.0.200.md +++ b/docs/release-notes/.FSharp.Compiler.Service/10.0.200.md @@ -3,6 +3,7 @@ * Type relations cache: optimize key generation ([Issue #19116](https://github.com/dotnet/fsharp/issues/18767)) ([PR #19120](https://github.com/dotnet/fsharp/pull/19120)) * Fixed QuickParse to correctly handle optional parameter syntax with `?` prefix, resolving syntax highlighting issues. ([Issue #11008753](https://developercommunity.visualstudio.com/t/F-Highlighting-fails-on-optional-parame/11008753)) ([PR #XXXXX](https://github.com/dotnet/fsharp/pull/XXXXX)) * Fix `--preferreduilang` switch leaking into `fsi.CommandLineArgs` when positioned after script file ([PR #19151](https://github.com/dotnet/fsharp/pull/19151)) +* Optimize empty string pattern matching to use null-safe .Length check instead of string equality comparison for better performance. * Fixed runtime crash when using interfaces with unimplemented static abstract members as constrained type arguments. ([Issue #19184](https://github.com/dotnet/fsharp/issues/19184)) ### Added diff --git a/src/Compiler/Checking/PatternMatchCompilation.fs b/src/Compiler/Checking/PatternMatchCompilation.fs index 0532182fb88..033f8572bbe 100644 --- a/src/Compiler/Checking/PatternMatchCompilation.fs +++ b/src/Compiler/Checking/PatternMatchCompilation.fs @@ -28,6 +28,9 @@ exception MatchIncomplete of bool * (string * bool) option * range exception RuleNeverMatched of range exception EnumMatchIncomplete of bool * (string * bool) option * range +// Debug flag for pattern match compilation - set to true to enable debug output +let verbose = false + type ActionOnFailure = | ThrowIncompleteMatchException | IgnoreWithWarning @@ -767,8 +770,18 @@ let (|ConstNeedsDefaultCase|_|) c = /// - Compact integer switches become a single switch. Non-compact integer /// switches, string switches and floating point switches are treated in the /// same way as DecisionTreeTest.IsInst. -let rec BuildSwitch inpExprOpt g expr edges dflt m = - if verbose then dprintf "--> BuildSwitch@%a, #edges = %A, dflt.IsSome = %A\n" outputRange m (List.length edges) (Option.isSome dflt) +let rec BuildSwitch inpExprOpt g isNullFiltered expr edges dflt m = + if verbose then + dprintf "BuildSwitch: isNullFiltered=%b, #edges=%d, dflt.IsSome=%b\n" isNullFiltered (List.length edges) (Option.isSome dflt) + edges |> List.iteri (fun i (TCase(discrim, _)) -> + let discrimStr = + match discrim with + | DecisionTreeTest.IsNull -> "IsNull" + | DecisionTreeTest.Const (Const.String s) -> sprintf "Const(String \"%s\")" s + | DecisionTreeTest.Const c -> sprintf "Const(%A)" c + | DecisionTreeTest.ArrayLength (n, _) -> sprintf "ArrayLength(%d)" n + | _ -> sprintf "%A" discrim + dprintf " Edge[%d]: %s\n" i discrimStr) match edges, dflt with | [], None -> failwith "internal error: no edges and no default" | [], Some dflt -> dflt @@ -780,11 +793,22 @@ let rec BuildSwitch inpExprOpt g expr edges dflt m = // In this case the 'expr' already holds the result of the 'isinst' test. | TCase(DecisionTreeTest.IsInst _, success) :: edges, dflt when Option.isSome inpExprOpt -> - TDSwitch(expr, [TCase(DecisionTreeTest.IsNull, BuildSwitch None g expr edges dflt m)], Some success, m) + TDSwitch(expr, [TCase(DecisionTreeTest.IsNull, BuildSwitch None g false expr edges dflt m)], Some success, m) // isnull and isinst tests | TCase((DecisionTreeTest.IsNull | DecisionTreeTest.IsInst _), _) as edge :: edges, dflt -> - TDSwitch(expr, [edge], Some (BuildSwitch None g expr edges dflt m), m) + // After an IsNull test, in the fallthrough branch (Some), we know the value is not null + let nullFiltered = match edge with TCase(DecisionTreeTest.IsNull, _) -> true | _ -> isNullFiltered + if verbose then + dprintf "IsNull/IsInst: edge type=%s, remaining edges=%d, setting nullFiltered=%b\n" + (match edge with | TCase(DecisionTreeTest.IsNull, _) -> "IsNull" | _ -> "IsInst") + (List.length edges) nullFiltered + edges |> List.iteri (fun i (TCase(d, _)) -> + let dStr = match d with + | DecisionTreeTest.Const (Const.String s) -> sprintf "Const(String \"%s\")" s + | _ -> sprintf "%A" d + dprintf " Remaining edge[%d]: %s\n" i dStr) + TDSwitch(expr, [edge], Some (BuildSwitch None g nullFiltered expr edges dflt m), m) // All these should also always have default cases | TCase(DecisionTreeTest.Const ConstNeedsDefaultCase, _) :: _, None -> @@ -792,14 +816,36 @@ let rec BuildSwitch inpExprOpt g expr edges dflt m = // Split string, float, uint64, int64, unativeint, nativeint matches into serial equality tests | TCase((DecisionTreeTest.ArrayLength _ | DecisionTreeTest.Const (Const.Single _ | Const.Double _ | Const.String _ | Const.Decimal _ | Const.Int64 _ | Const.UInt64 _ | Const.IntPtr _ | Const.UIntPtr _)), _) :: _, Some dflt -> + if verbose then dprintf "foldBack: Processing %d edges with isNullFiltered=%b\n" (List.length edges) isNullFiltered List.foldBack (fun (TCase(discrim, tree)) sofar -> + if verbose then + let discrimStr = + match discrim with + | DecisionTreeTest.Const (Const.String s) -> sprintf "Const(String \"%s\")" s + | DecisionTreeTest.Const c -> sprintf "Const(%A)" c + | DecisionTreeTest.ArrayLength (n, _) -> sprintf "ArrayLength(%d)" n + | _ -> sprintf "%A" discrim + dprintf " foldBack iteration: discrim=%s, isNullFiltered=%b\n" discrimStr isNullFiltered let testexpr = expr let testexpr = match discrim with | DecisionTreeTest.ArrayLength(n, _) -> + if verbose then dprintf " ArrayLength: creating test with isNullFiltered=%b\n" isNullFiltered + let _v, vExpr, bind = mkCompGenLocalAndInvisibleBind g "testExpr" m testexpr + // Skip null check if we're in a null-filtered context + let test = mkILAsmCeq g m (mkLdlen g m vExpr) (mkInt g m n) + let finalTest = if isNullFiltered then test else mkLazyAnd g m (mkNonNullTest g m vExpr) test + if verbose then dprintf " ArrayLength: skipping null check? %b\n" isNullFiltered + mkLetBind m bind finalTest + | DecisionTreeTest.Const (Const.String "") -> + // Optimize empty string check to use null-safe length check let _v, vExpr, bind = mkCompGenLocalAndInvisibleBind g "testExpr" m testexpr - mkLetBind m bind (mkLazyAnd g m (mkNonNullTest g m vExpr) (mkILAsmCeq g m (mkLdlen g m vExpr) (mkInt g m n))) + let test = mkILAsmCeq g m (mkGetStringLength g m vExpr) (mkInt g m 0) + // Skip null check if we're in a null-filtered context + let finalTest = if isNullFiltered then test else mkLazyAnd g m (mkNonNullTest g m vExpr) test + if verbose then dprintf " Empty string: skipping null check? %b\n" isNullFiltered + mkLetBind m bind finalTest | DecisionTreeTest.Const (Const.String _ as c) -> mkCallEqualsOperator g m g.string_ty testexpr (Expr.Const (c, m, g.string_ty)) | DecisionTreeTest.Const (Const.Decimal _ as c) -> @@ -1152,7 +1198,7 @@ let CompilePatternBasic // OK, build the whole tree and whack on the binding if any let finalDecisionTree = let inpExprToSwitch = (match inpExprOpt with Some vExpr -> vExpr | None -> GetSubExprOfInput subexpr) - let tree = BuildSwitch inpExprOpt g inpExprToSwitch simulSetOfCases defaultTreeOpt mMatch + let tree = BuildSwitch inpExprOpt g false inpExprToSwitch simulSetOfCases defaultTreeOpt mMatch match bindOpt with | None -> tree | Some bind -> TDBind (bind, tree) diff --git a/src/Compiler/TypedTree/TypedTreeOps.fsi b/src/Compiler/TypedTree/TypedTreeOps.fsi index a68ffaf8af1..78f476f971f 100755 --- a/src/Compiler/TypedTree/TypedTreeOps.fsi +++ b/src/Compiler/TypedTree/TypedTreeOps.fsi @@ -2348,6 +2348,8 @@ val mkIncr: TcGlobals -> range -> Expr -> Expr val mkLdlen: TcGlobals -> range -> Expr -> Expr +val mkGetStringLength: TcGlobals -> range -> Expr -> Expr + val mkLdelem: TcGlobals -> range -> TType -> Expr -> Expr -> Expr //------------------------------------------------------------------------- diff --git a/tests/FSharp.Compiler.ComponentTests/EmittedIL/Inlining/EmptyStringPattern.fs b/tests/FSharp.Compiler.ComponentTests/EmittedIL/Inlining/EmptyStringPattern.fs new file mode 100644 index 00000000000..ad0c2967690 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/EmittedIL/Inlining/EmptyStringPattern.fs @@ -0,0 +1,28 @@ +module TestLibrary + +let inline classifyString (s: string) = + match s with + | "" -> "empty" + | null -> "null" + | _ -> "other" + +let inline testEmptyStringOnly (s: string) = + match s with + | "" -> 1 + | _ -> 0 + +let inline testBundledNullAndEmpty (s: string) = + match s with + | null | "" -> 0 + | _ -> 1 + +let inline testBundledEmptyAndNull (s: string) = + match s with + | "" | null -> 0 + | _ -> 1 + +// Usage functions to show inlining in action +let useClassifyString s = classifyString s +let useTestEmptyStringOnly s = testEmptyStringOnly s +let useBundledNullAndEmpty s = testBundledNullAndEmpty s +let useBundledEmptyAndNull s = testBundledEmptyAndNull s diff --git a/tests/FSharp.Compiler.ComponentTests/EmittedIL/Inlining/EmptyStringPattern.fs.il.bsl b/tests/FSharp.Compiler.ComponentTests/EmittedIL/Inlining/EmptyStringPattern.fs.il.bsl new file mode 100644 index 00000000000..0d1c3c137a5 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/EmittedIL/Inlining/EmptyStringPattern.fs.il.bsl @@ -0,0 +1,276 @@ + + + + + +.assembly extern runtime { } +.assembly extern FSharp.Core { } +.assembly assembly +{ + .custom instance void [FSharp.Core]Microsoft.FSharp.Core.FSharpInterfaceDataVersionAttribute::.ctor(int32, + int32, + int32) = ( 01 00 02 00 00 00 00 00 00 00 00 00 00 00 00 00 ) + + + + + .hash algorithm 0x00008004 + .ver 0:0:0:0 +} +.module assembly.exe + +.imagebase {value} +.file alignment 0x00000200 +.stackreserve 0x00100000 +.subsystem 0x0003 +.corflags 0x00000001 + + + + + +.class public abstract auto ansi sealed TestLibrary + extends [runtime]System.Object +{ + .custom instance void [FSharp.Core]Microsoft.FSharp.Core.CompilationMappingAttribute::.ctor(valuetype [FSharp.Core]Microsoft.FSharp.Core.SourceConstructFlags) = ( 01 00 07 00 00 00 00 00 ) + .method public static string classifyString(string s) cil managed + { + + .maxstack 8 + IL_0000: nop + IL_0001: ldarg.0 + IL_0002: brfalse.s IL_0010 + + IL_0004: ldarg.0 + IL_0005: callvirt instance int32 [runtime]System.String::get_Length() + IL_000a: ldc.i4.0 + IL_000b: ceq + IL_000d: nop + IL_000e: br.s IL_0012 + + IL_0010: ldc.i4.0 + IL_0011: nop + IL_0012: brtrue.s IL_0019 + + IL_0014: ldarg.0 + IL_0015: brfalse.s IL_001f + + IL_0017: br.s IL_0025 + + IL_0019: ldstr "empty" + IL_001e: ret + + IL_001f: ldstr "null" + IL_0024: ret + + IL_0025: ldstr "other" + IL_002a: ret + } + + .method public static int32 testEmptyStringOnly(string s) cil managed + { + + .maxstack 8 + IL_0000: nop + IL_0001: ldarg.0 + IL_0002: brfalse.s IL_000e + + IL_0004: ldarg.0 + IL_0005: callvirt instance int32 [runtime]System.String::get_Length() + IL_000a: brtrue.s IL_000e + + IL_000c: ldc.i4.1 + IL_000d: ret + + IL_000e: ldc.i4.0 + IL_000f: ret + } + + .method public static int32 testBundledNullAndEmpty(string s) cil managed + { + + .maxstack 8 + IL_0000: nop + IL_0001: ldarg.0 + IL_0002: brfalse.s IL_0017 + + IL_0004: ldarg.0 + IL_0005: brfalse.s IL_0013 + + IL_0007: ldarg.0 + IL_0008: callvirt instance int32 [runtime]System.String::get_Length() + IL_000d: ldc.i4.0 + IL_000e: ceq + IL_0010: nop + IL_0011: br.s IL_0015 + + IL_0013: ldc.i4.0 + IL_0014: nop + IL_0015: brfalse.s IL_0019 + + IL_0017: ldc.i4.0 + IL_0018: ret + + IL_0019: ldc.i4.1 + IL_001a: ret + } + + .method public static int32 testBundledEmptyAndNull(string s) cil managed + { + + .maxstack 8 + IL_0000: nop + IL_0001: ldarg.0 + IL_0002: brfalse.s IL_0010 + + IL_0004: ldarg.0 + IL_0005: callvirt instance int32 [runtime]System.String::get_Length() + IL_000a: ldc.i4.0 + IL_000b: ceq + IL_000d: nop + IL_000e: br.s IL_0012 + + IL_0010: ldc.i4.0 + IL_0011: nop + IL_0012: brtrue.s IL_0017 + + IL_0014: ldarg.0 + IL_0015: brtrue.s IL_0019 + + IL_0017: ldc.i4.0 + IL_0018: ret + + IL_0019: ldc.i4.1 + IL_001a: ret + } + + .method public static string useClassifyString(string s) cil managed + { + + .maxstack 8 + IL_0000: nop + IL_0001: ldarg.0 + IL_0002: brfalse.s IL_0010 + + IL_0004: ldarg.0 + IL_0005: callvirt instance int32 [runtime]System.String::get_Length() + IL_000a: ldc.i4.0 + IL_000b: ceq + IL_000d: nop + IL_000e: br.s IL_0012 + + IL_0010: ldc.i4.0 + IL_0011: nop + IL_0012: brtrue.s IL_0019 + + IL_0014: ldarg.0 + IL_0015: brfalse.s IL_001f + + IL_0017: br.s IL_0025 + + IL_0019: ldstr "empty" + IL_001e: ret + + IL_001f: ldstr "null" + IL_0024: ret + + IL_0025: ldstr "other" + IL_002a: ret + } + + .method public static int32 useTestEmptyStringOnly(string s) cil managed + { + + .maxstack 8 + IL_0000: nop + IL_0001: ldarg.0 + IL_0002: brfalse.s IL_000e + + IL_0004: ldarg.0 + IL_0005: callvirt instance int32 [runtime]System.String::get_Length() + IL_000a: brtrue.s IL_000e + + IL_000c: ldc.i4.1 + IL_000d: ret + + IL_000e: ldc.i4.0 + IL_000f: ret + } + + .method public static int32 useBundledNullAndEmpty(string s) cil managed + { + + .maxstack 8 + IL_0000: nop + IL_0001: ldarg.0 + IL_0002: brfalse.s IL_0017 + + IL_0004: ldarg.0 + IL_0005: brfalse.s IL_0013 + + IL_0007: ldarg.0 + IL_0008: callvirt instance int32 [runtime]System.String::get_Length() + IL_000d: ldc.i4.0 + IL_000e: ceq + IL_0010: nop + IL_0011: br.s IL_0015 + + IL_0013: ldc.i4.0 + IL_0014: nop + IL_0015: brfalse.s IL_0019 + + IL_0017: ldc.i4.0 + IL_0018: ret + + IL_0019: ldc.i4.1 + IL_001a: ret + } + + .method public static int32 useBundledEmptyAndNull(string s) cil managed + { + + .maxstack 8 + IL_0000: nop + IL_0001: ldarg.0 + IL_0002: brfalse.s IL_0010 + + IL_0004: ldarg.0 + IL_0005: callvirt instance int32 [runtime]System.String::get_Length() + IL_000a: ldc.i4.0 + IL_000b: ceq + IL_000d: nop + IL_000e: br.s IL_0012 + + IL_0010: ldc.i4.0 + IL_0011: nop + IL_0012: brtrue.s IL_0017 + + IL_0014: ldarg.0 + IL_0015: brtrue.s IL_0019 + + IL_0017: ldc.i4.0 + IL_0018: ret + + IL_0019: ldc.i4.1 + IL_001a: ret + } + +} + +.class private abstract auto ansi sealed ''.$TestLibrary + extends [runtime]System.Object +{ + .method public static void main@() cil managed + { + .entrypoint + + .maxstack 8 + IL_0000: ret + } + +} + + + + + diff --git a/tests/FSharp.Compiler.ComponentTests/EmittedIL/Inlining/Inlining.fs b/tests/FSharp.Compiler.ComponentTests/EmittedIL/Inlining/Inlining.fs index ffd75e0d081..0e7e1aa84e3 100644 --- a/tests/FSharp.Compiler.ComponentTests/EmittedIL/Inlining/Inlining.fs +++ b/tests/FSharp.Compiler.ComponentTests/EmittedIL/Inlining/Inlining.fs @@ -229,3 +229,10 @@ printfn $"{(SecondType ()).SecondMethod()}" |> compileAndRun |> shouldSucceed + + // Test empty string pattern optimization in inlining + [] + let ``EmptyStringPattern_fs`` compilation = + compilation + |> getCompilation + |> verifyCompilation diff --git a/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs b/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs index 7ebd99b4671..8dbdf763f4d 100644 --- a/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs +++ b/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs @@ -314,4 +314,37 @@ let TestTwoColumnsOfTypeTestsWithSealedTypes(x: obj, y: obj) = IL_00d9: ret } """ - ] \ No newline at end of file + ] + + [] + let ``Test codegen for empty string pattern with null safety``() = + FSharp """ +module Test +let TestEmptyStringPattern(x: string) = + match x with + | "" -> "empty" + | _ -> "other" + """ + |> compile + |> shouldSucceed + |> verifyIL [ + """ + .method public static string TestEmptyStringPattern(string x) cil managed + { + + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: brfalse.s IL_0011 + + IL_0003: ldarg.0 + IL_0004: callvirt instance int32 [runtime]System.String::get_Length() + IL_0009: brtrue.s IL_0011 + + IL_000b: ldstr "empty" + IL_0010: ret + + IL_0011: ldstr "other" + IL_0016: ret + } +""" + ] \ No newline at end of file