Optimize 'string', remove boxing where possible, prevent FS0670, simplify generic code (RFC-1089)

[abelbraaksma]

Fixes #7958
Fixes #9153
Depends on: #9622 edit 7/21, now merged.

Tentatively implements: https://github.com/fsharp/fslang-design/blob/master/FSharp-5.0/FS-1089-allow-string-everywhere.md

TLDR: after this change, string FS0670 will not be raised anymore, while code is still inlined using compiler optimization syntax. Speed is much improved and generated IL and disassembly much smaller. Less boxing and less memory pressure.

Before this change:

• Each call to string led to exactly the same inlined code, as found by @teo-tsirpanis in The 'string' operator produces suboptimal code when the object's type is known. #9153.
• There was an abundance of dead/unreachable code in string
• Not possible to use string X if X was of a generic type specified on the class/record/struct/du, raises FS0670, reported by me in Using string obj vs obj.ToString() leads to problems: the type parameter can escape its scope #7958.
• It was slooow compared to calling ToString() directly
• Boxing created unnecessary Gen-0 pressure

After this change

• Function can stay 'inline', but will not have SRTP anymore
• Optimized IL code for known system types
• Call gets erased for string types
• Uses call instead of callvirt for types like float, float32, decimal (no boxing)
• Uses callvirt with constrained for char and bool (no boxing) and the integral unsigned types (4 integers)
• Uses boxing with callvirt for the integral types (8 integers) because of backward compat issue with enums
• EDIT: Uses boxing with callvirt for the integral signed types (4 integers), this is essentially the old behavior.
• Uses direct ToString() for a few known sealed types
• Prevents boxing for some "large" known value types like DateTime, Guid, which improves perf by some percentage points.
• Dead code removed
• Unnecessary protective shadow-copying is removed

Code generation example

When having let fromFloat (x: float) = string x, before it looked like this:

/// Generated code BEFORE this change
public static string fromFloat(double x) {
    object obj = x;
    if (obj != null)
    {
        IFormattable formattable = obj as IFormattable;
        if (formattable != null)
        {
            IFormattable formattable2 = formattable;
            return formattable2.ToString(null, CultureInfo.InvariantCulture);
        }
        object obj2 = obj;
        return obj2.ToString();
    }
    return "";
}

After this change, it's this:

/// Generated code AFTER this change
public static string fromFloat(double x) {
     return x.ToString(null, CultureInfo.InvariantCulture); 
}

Performance effects

Important note, likely the timings will be better, as this was with the new version using debug jit, vs original using optimized jit, see BDN bug dotnet/BenchmarkDotNet#1493.

EDIT: New charts, as the change has now evolved and it is much better

Across the board: increase in speed up to 10x, typically around 20-30%. Less allocations, about 40% on average where it applies, sometimes down to zero.

Performance of numeric types

A note on numeric types: the unsigned types use ToString(), which is faster and works correctly with Enum. The signed types use IFormattable::ToString(null, InvariantCulture), because there's the off-chance that the minus sign is defined in "current culture", and we cannot use that. Hence, the speed increase is larger for the unsigned types.

Other numeric types use the fastest path possible, which means a constrained callvirt on ToString() or a call on the proper variant of IFormattable::ToString.

Performance of char, bool, enum

The numbers speak for themselves. Note the 10x improvement for bool and the fact that there are now zero allocations in that case:

Performance of other types

Below the improvement of string (gets erased, so 0.000ns!), Guid, DateTime a simple custom DU with a ToString override, and a type that is already cast to IFormattable.

[abelbraaksma]

Expected: ["type OperatorTestsByte";
"let testByteEqualsOperator(e1) (e2) = Operators.op_Equality<Microsoft.FSharp.Core.byte> (e1,e2) @ (4,63--4,72)";
"let testByteNotEqualsOperator(e1) (e2) = Operators.op_Equality<Microsoft.FSharp.Core.bool>(Operators.op_Equality<Microsoft.FSharp.Core.byte> (e1,e2),False) @ (5,63--5,73)";

That's unexpected! What are these tests? Doesn't sound like they rely on string in any way. @KevinRansom, any idea?

[abelbraaksma]

It also throws this:

Cannot print exception string because Exception.ToString() failed.

Which in turn appears to be a bug in the runtime, or at least a resurfacing of a bug, possibly related are:

• https://stackoverflow.com/questions/5634417/caught-exception-is-null-itself?noredirect=1&lq=1
• Exception.ToString() failed. runtime#5203
• Cannot print exception string because Exception.ToString() failed runtime#6299
• [ARM/Linux] Regression in Unittest - Exception.ToString() failed runtime#6283
• https://devblogs.microsoft.com/oldnewthing/20151222-00/?p=92691 (but for Raymond Chen it disappeared by itself, just not for me)
• In Paket is was caused by non-resolved assembly: dotnet run fails with "Unhandled exception" when paket manages the dependencies fsprojects/Paket#3519

Reading all these reports, this warrants some research/debugging etc. It looks like it's caused by something on the stack being null or unknown somehow.

[abelbraaksma]

Ok, that last error comes from the temp exe that is created, which, when run in isolation, shows the actual error:

An unhandled exception of type 'System.IO.FileNotFoundException' occurred in Unknown Module.
Could not load file or assembly 'System.Runtime, Version=4.2.2.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The system cannot find the file specified.

If I load that assembly in ILSpy, it says:

// System.Runtime, Version=4.2.2.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a
// Assembly reference loading information:
// Info: Success - Found in Assembly List

The generated assembly: hkch4czt.2ka.zip

/cc @KevinRansom, help?

Looks like I need to do "something" when I change the signature of a public function, in this case string, even though the actual signature doesn't change (it just changes from inline to one without it, as discussed with @dsyme a few months ago).

Here's what I think happens:

• Test is C# simple with static operator method - Runs
• The test creates a compiled exe in F# and compiled dll in C#
• This is put in the temp directory
• The exe is run, an exit code of non-zero triggers an Assert.Fail. That method itself fails, because it is called with a null reference from the stderr stream. Not a big deal, above is the underlying error.
• The underlying error is caused by an FSharp.Core reference, which should reference the now-changed FSharp.Core, but it seems like it doesn't, it takes the installed FSharp.Core (I think).

Is this how these tests are supposed to work? In particular, the last step, is it possible that it doesn't use the newly compiled FSharp.Core.dll? For sure it isn't in the temp output dir that the test uses (see the attached zip).

[cartermp]

Those parts of the FCS tests are horribly annoying and not very simple to fix. The FCS tests also don't use the built FSharp.Core since FCS is currently designed to be compatible with dowlevel FSharp.Core binaries. I personally don't like that behavior, but it is what it is.

[abelbraaksma]

The FCS tests also don't use the built FSharp.Core since FCS is currently designed to be compatible with dowlevel FSharp.Core binaries

Thanks, @cartermp, I just noticed that the actual tests that were failing here had zero dependency on FSharp.Core. Since they used string, which used to be inlined, that was not a problem: they never called the actual Operators.string. I've now changed these calls to ToString() instead, so that I don't have to change the whole config of these tests to use the current FSharp.Core.

The other ones, from this comment above (#9549 (comment)), I've no idea yet, as I don't see them locally (or I'm doing something wrong). I'm now running all tests locally again, see if I missed something.

[abelbraaksma]

@KevinRansom, in our discussion on this, you asked if we could improve, esp. to prevent excessive boxing. My take on this at the time was: no, we can't combine statically-resolvable type compiler syntax and at the same be non-inline (to prevent FS0670 from throwing).

But after some experimenting, and searching for uses in the source code of the (# ..#) inline IL syntax and static when clauses, it turns out that the compiler understands what to do with normal generic types like 'T and will inline when you give it an instruction to do so on a specific path, regardless of the function being marked inline or not. Pretty amazing and powerful stuff! This syntax may help in some other scenarios as well.

Since this idea makes this change larger than originally intended, I decided to go the "long" route and write out an RFC (see fsharp/fslang-design#479) and the corresponding lang suggestion (see fsharp/fslang-suggestions#890), which basically backs the findings of several bug reports. It's a minimal change in the surface area, but a change after all, hence the RFC.

So where does that leave us in this PR? Well, in short, we now have the best of both worlds:

• string is no longer inline, it won't throw FS0670 anymore (it still shows it in the editor, but it compiles fine now).
• IL generation for all basic struct types is now without copying, no boxing
• IL generation for integral numeric types still requires boxing, because these can be used for Enum and need to be runtime-tested for IFormattable. It would be nice if I could distinguish statically between an enum and an int, but there's presently no way that works.
• I removed null-checks for other structs, as structs cannot be null (the compiler still emits a redundant shadow copy, though in other places I managed to remove these)
• Added no-op for string and fast-paths for decimal, char and a few others. You may be surprised to see Guid and DateTime there, but these are large and common structs and perform about 2-3x faster without the boxing.

All in all, I'm pretty happy with the result in terms of generated IL. Maybe @teo-tsirpanis could also have a second-opinion look at this, as he reported the "bad IL" issue (#9153).

Note: the baseline tests will still fail, but I do not want to start working on these unless we're all confident this is the way to go.

[teo-tsirpanis]

I am still worried by the excessive use of the box operator, but I remember that those who care about their code's performance will directly call the ToString overload they want.

The core of the issue is that FS0670 is overly restrictive and prevents us from doing sensible things.

My opinion would be to loosen the aforementioned restriction and in the meantime, use the string implementation that would be the best if we assume we are not burdened by FS0670.

As for the changes at hand, they seem better than the previous implementation of the string operator (in particular the elimination of the extra null check), but the impact of the boxing elimination for structs is not that great, given that some of the most commonly used structs and all custom ones will get eventually boxed.

I am wondering what makes the enums such a special case. I wrote the following C# program

using System;

namespace ToStringTest
{
    static class Program
    {
        static string AnyToString<T>(T x) => x.ToString();
        static void Main()
        {
            Console.WriteLine(AnyToString(521));
            Console.WriteLine(AnyToString(ConsoleKey.U));
        }
    }
}

and the results are the expected ones (521 and U). A straightforward call to ToString on a generic type will correctly deduce we are talking about an enum. I understand that the compiler cannot currently distinguish between an integer and an enum backed by an integer, but does a case like when 'T: Enum = value.ToString() work?

[abelbraaksma]

@teo-tsirpanis, thanks for looking into this. It compiles with Enum, but it doesn't work, unfortunately. The reason is that Enum is an abstract class, so it will never match.

The problem in F# is that when you use static type checks, any int an an enum if that type, are considered equal. The easiest solution would be to drop printing the name of the enum case.

C#, since 7.3, can distinguish between enum and integral types. That's a fairly recent feature. There's not yet a similar proposal for F#, and I don't know how hard it would be to implement (looking at the C# discussions, it wasn't trivial). I assume that's also the reason why your C# example works. You're example is, however, not comparable (runtime generics vs static generics), and because the equivalent C# method would be one that tests for IFormattable. I don't know if that's simple to express in C# -- and without boxing.

What I haven't tried, though, is if calling ToString on either an integral type or enum would have the same result. But then, still, we need to tell the CLR what type it is, and we end up where we started.

I'll think about it some more.

Btw, it's more than just the null check, that's gone. It's also call vs callvirt, and the second null check (there's no check at all anymore). Casting without boxing is not possible, and calling IFormattable.ToString without casting is not possible. That's the conundrum.

Note that in the original, none of these special cases were hit, ever. For instance, using it on a decimal, is now equal to doing 42.42M.ToString(...) directly. The function is inlined for that, and all special cases.

[abelbraaksma]

I'll also run some benchmarks. It was already faster, but should be even better now, even for integral types (the ones that are still boxed). The others are as fast as they would be if you'd code it by hand.

[abelbraaksma]

@teo-tsirpanis, maybe there's a solution after all, even for the last cases that still need boxing now. I was stuck on the idea that we must use IFormattable. The reason for this is that we want to pass InvariantCulture. However, for integral types (the 8 ints) and for enums, this has no effect.

If we look at the implementation of Int32.ToString() it does indeed use CurrentCulture. There are two problems with this: (1) it used to be much slower than InvariantCulture and (2) we cannot ascertain that the output will be equal always for all cultures.

I think that, if the recent rewrites of all ToString() functions of the BCL types for .NET Core have had the effect that it is equally fast, we can go with that. Otherwise, the boxing is a small price to pay for those tiny types, compared to the loads of work that is done for converting them to a string.

The idea is to change the 8 integral types to look like this, which compiles, and correctly prints the names of enums:

when 'T : int32      = (# "" value : 'T #).ToString()

I'll do some more testing and create benchmarks for comparison. At least in the past this was much slower than using InvariantCulture, let's see how it is now.

[abelbraaksma]

@teo-tsirpanis, I've given this a little bit more thought. Your C# example works locally by using ToString(), but, as mentioned, that won't force the InvariantCulture (why on earth that isn't the default on any ToString() in the BCL is beyond me, but that's a fact of life).

Consider this:

Then click Apply and reset your FSI. This will give:

> (-12).ToString();;
val it : string = "_12"

> string -12;;
val it : string = "-12"

Or with "over line", which apparently is used in some educational systems, so it isn't entirely impossible that this happens in the wild:

> (-12).ToString();;
val it : string = "‾12"

Conclusion: as much as I would like to use ToString(), we can't (unless @cartermp/@KevinRansom would jump in and say: "this is so rare, just use ToString() for integral types and be done with it.").

As far as I know it is not possible to convert to IFormattable and prevent boxing (I tried, against better knowing, it crashes horribly). The box-less versions for float work, however, because it is possible to issue a call instruction, since the type is known. For ints, the type is not known, I would need to issue a call instruction on either the int, or the concrete enum. Technically, this information is available during type checking, but not in the stdlib.

Result of all this research, I have to box-unbox and use callvirt. Using just callvirt on a stack-allocated type is not possible (the CLR let's you do that, but you'll invariably receive a runtime NRE).

Here's how the current approach looks if it were coded in C# for uint:

return ((IFormattable) ((uint) x)).ToString(null, CultureInfo.InvariantCulture);

Or if it was an enum:

return ((IFormattable) x).ToString(null, CultureInfo.InvariantCulture);

EDIT: I've added a language request to allow such syntax. If I knew where in the code this is, I might implement it. Until then, I think we'll have to live with the 90% rule: almost everything is optimized now, the remaining stuff will have to be postponed. See #9594.

[abelbraaksma]

@KevinRansom, I think this is now "as good as it gets". I've updated the original text of the PR and updated the performance charts (as you requested). It's certainly much better now than my first attempt: it's faster, less allocations and better IL code optimizations.

If you think this is good enough to get in, I'll set to work on the baseline tests (or maybe you can help there?).

[dsyme]

@abelbraaksma Why not continute to inline the string function, even if SRTP is no longer used? I don't understand why the inline has to be removed.

that we remove inline from the Operators.string function, but without losing the 'inlinability'. This allows its usage in places where you would now get an error that it "escapes its scope" (report: #7958). Consider:

I believe it is the removal of SRTP that solves this problem for you, rather than the removal of inline.

Could you try with the inline re-established please?

Thanks

[cartermp]

Since this involved some surprisingly complicated details that @TIHan and @dsyme talked about extensively I'll defer to them on this PR.

[abelbraaksma]

Ping @TIHan, @dsyme. can either or both please review?

As mentioned above, I think this is now "as good as it can get" with current features of the language, and improves performance of string by a large margin, plus emits the proper/expected IL.

For the signed-int case, which needs a distinction between enum and intX, this requires a new feature for the static when optimization syntax. I propose to merge this as-is and take the discussion on such improvements into a new thread, as they have wider applicability.

[abelbraaksma]

Ping @TIHan, @dsyme. can either or both please review?

/cc @TIHan, @dsyme, 🙏? ;)

[cartermp]

@abelbraaksma currently we're in a very hard push to finish off and review open type declarations and string interpolation. We're trying to get those completed by the end of this week or maybe the end of next week. After that we should be more "free" :)

[abelbraaksma]

@cartermp Cool, thanks for the update! If I can help in any way with those tasks (tests or otherwise), ping me :)

[cartermp]

If you're up for it, our biggest concern right now is ensuring that tooling works well with string interpolation. If you're able to pull down that branch, build a new VS, and use F# scripts to play with the feature and give feedback then that would be great.

Unfortunately it's tricky to use the feature with a real codebase because it relies on preview FSharp.Core functionality that is only accessible in F# scripts or some trickery where a project has its FSharp.Core reference swapped out for the built FSharp.Core. But if you use F# scripts it's fine, which I've been doing.

[abelbraaksma]

@cartermp Yeah, I know how to cheat a different Core reference (used it in benchmarking for before/after in one run), and how to switch the fsc.exe, I'll give it a shot with some scripts and projects I have.

[dsyme]

I came to review this but there are a lot of unrelated changes in the diff in github

Anyone know what's up?

[abelbraaksma]

Anyone know what's up?

@dsyme It may have something to do with introduction of main? This started from master? Hmm. I see the same as you, a lot of these changes were certainly not by me. I can try to rebase, see if that helps clear things up.

Most certainly, last time I checked this, it was just a handful of changes.

[abelbraaksma]

@dsyme, nothing a little merge couldn't fix. There are now just 5 files changes, instead of the 67 you saw. Strange, but I do think it was caused by main-renaming, since there were zero conflicts in the merge.

[dsyme]

@dsyme, nothing a little merge couldn't fix. There are now just 5 files changes, instead of the 67 you saw. Strange, but I do think it was caused by main-renaming, since there were zero conflicts in the merge.

Thanks! :-)

[abelbraaksma]

PS: I'm live on Slack if you need/want to discuss a few things.

[abelbraaksma]

That was fast ;). Tx!

Then there's only @TIHan left as pending review, @cartermp, is that still necessary? Or @TIHan, if you're online, could you do the honors?

[cartermp]

This is a good change. I like the additional test coverage, and the results in benchmarking also speak for themselves. Great work!

[cartermp]

I would still like @TIHan to take a look here, as this did spark a lengthy conversation about these kinds of optimizations and the things we can and cannot do.

[TIHan]

This is good; we discussed this before.

It does improve things, but it's not the perfect solution - perfect solution requires metadata format changes which I don't think we are willing to accept at the moment; therefore, we can take in this change.