-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make F# string functions fast and AOT/linker friendly #919
Comments
I like this a lot. Ideally, there wouldn't be any Undoubtedly, there'll be corner cases where we may have to resort to old style reflection to prevent backwards compatibility issues, but hopefully not many. |
So if I understand this correctly (see below), parts 1 and 2+3 could be worked on separately, with completely independent RFCs? Part 1 - Compiler emitting specialized Part 2 + 3 - Compiler emitting specialized formatted printing instructions every time it encounters An official verdict would be nice here. |
I'm ok with approving this though compat may be very tricky. I don't think it needs an RFC.
These are bugs in the .NET native systems - TBH I'm not a fan of buggy and incomplete implementations of things calling themselves .NET that are not .NET, and feeel it's a bit of a waste of time trying to chase them down. That said, it makes sense to make the codegen independent of printf for performance reasons alone. :) |
Can you please elaborate on this? I somewhat naively thought the only tricky part would be preserving the same indentation. |
The problem is that in theory the recursive structural calls for printing would have to do what We could consider a breaking change and simply call ToString() recursively. However that might bite us quite badly - e.g. %A handles deep and recursive object graphs |
We could use an |
Hopefully some shortcuts that result in cleaner code will be allowed, but we probably would need some degree of going into object graphs. I believe none of the string printing functions ever had a spec so are full of inconsistencies and some of them might need cleaning up in the process. For fidelity:
ValueSome(0, (None:int option)).ToString() // "(0, )" - should be "ValueSome(0, None)"
[Some 0; None].ToString() // "[Some(0); null]"
sprintf "%A" [Some 0; None] // "[Some 0; None]"
(0, (None: int option)).ToString() // "(5, )"
sprintf "%A" (0, (None:int option)) // "(5, None)" So sometimes |
@Happypig375, that part's not the problem. Every time a record has another record in a field, the indentation goes up. type R = { S: int; Z: R option }
let r = { S = 3; Z = Some { S = 2; Z = Some { S = 5; Z = None } } }
r.ToString()
(*
{ S = 3
Z = Some { S = 2
Z = Some { S = 5
Z = None } } }
*) So you can't just call
I think the solution would be to provide another |
We can split line breaks and indent. |
That's going to work until you encounter a field containing a multiline string :). |
Then you have to think about whether sudden unindentations like that are visually appealing. |
The behaviour of '%A' itself is now better documented, including some of the inconsistencies. https://docs.microsoft.com/en-us/dotnet/fsharp/language-reference/plaintext-formatting#a-formatting . However it's just not a spec that can be implemented in compositional fragments by using However the problem isn't so much the spec/not spec, it's that code may depend on existing behaviour regardless. We hit this recently when we tried to change That said I'm guessing that the compat issue would be manageable by making sure all primitive non-nested records/unions print identically. However if not, another option might be to have an attribute that guides the generation of a printf-free |
Can you mark the issue approved in principle then please?
Yes @dsyme , @Happypig375 and @kerams have raised indentation as a difficult problem. I propose dispensing with line breaks altogether (for // This logic is simple and only needs to know about ToString() on inner elements. (Use knowledge that the inner thing is a record to avoid extra brackets.)
{ S = 3; Z = Some { S = 2; Z = Some { S = 5; Z = None } } }
// Similarly this will also work without knowing more than ToString() on inners.
{ S = 3 // single-line
Z =
Some // multi-line inner record string, so put it all on separate lines and indent together
{
S = 2
Z =
Some
{
S = 5
Z = None
}
}
} I prefer the first version with a single line. |
I don't really understand the technical proposal here. I believe replacing the default ToString implementations just won't be possible without breaking compat - that is, in some sense the existing object-to-layout engine in FSharp.Core will need to be used, though maybe not via May I ask - why not just have the user who cares it implement their own ToString on each type? |
Don, are you serious? Just so that you know how painful this is, take a look at our workaround's diff: (The important bits are in FSharpUtil.fs, the rest are a consequence of that, but as you see, we've had to implement a task for CI that detects when people use sprintf/printf/failwithf and pokes the developer to use our ugly SPrintF1,SPrintF2,...,SPrintFn workarounds instead.) |
(Not to mention that any F# dependencies we have, we gotta create our own forks that don't use sprintf/printf/failwithf...) It doesn't scale. We actually de-prioritised UWP because of this nightmare. |
Well, it's painful because you were running on an incomplete .NET implementation. I'm not surprised you de-prioritised it. More positively, I can see the advantages of allowing people a path to tighten up their code to be free of implicit reflection, much as with implicit boxing. What's a practical path to that without bifurcation? Would an assembly-level attribute that turns off |
Not sure why you deleted this part of your message, but the thing is, me, as a ISV, how do I deal with this issue? I cannot publish an F# app on the Windows Store. I don't care about UWP, I just need something that allows me to publish to the WindowsStore. And fixing this github issue would allow me to. |
I don't really know, sorry. My vague understanding is that other routes have since opened up for publishing to Windows Store using either JS front-ends (use Fable?) or sandboxed .NET Framework+WPF or something, but I could be wrong. Were your workarounds to lint for problematic constructs sufficient? If the use of |
We're not 100% sure because we de-prioritized the effort. After committing the workaround commit you saw, we still had issues but didn't have time to investigate if they come from the fact that our dependencies still use printf/sprintf/failwithf, or from other AOT-related problems; because finding a workaround for the dependencies problems is too time consuming. However, fixing this github issue (to make FSharp.Core not use reflection for the simplest cases) would be much simpler. |
Having to rewrite my frontend which already works in so many platforms (Android, iOS, macOS, Linux)* with a single codebase? Quite ironic that using .NET one cannot really support the Windows platform... thanks F# :'-( * thanks to XamarinForms (soon Maui) |
This is the never-ending saga of the awfulness that is the Windows Store and Win10 apps 😢. It's unsurprising to see that 5+ years later, it's still as terrible as ever. I hear that at some point they're going to allow actual, bona fide windows apps running against a real .NET implementation into the store. Or so I was told year after year, and yet that never came. My honest advice is to abandon Windows app development for the store in general. It is not a stable platform. |
If the criterion of making real apps is that the user's machine has to JIT compile the code, I am happy to deliver fake apps that load quickly. Even the F# team is not completely indifferent to the performance benefits of AOT. Of late this thread has got confused and even if we ignore everything except the comments of @dsyme it is not easy to follow. Let me try to piece this together.
Can we know how to progress? Either mark approved in principle (so work can be started with some flexibility in the degree of mimicing of existing behavior) or require an RFC if it needs fully speccing first.
Yes we are agreed on this. Printing of simple union cases is much more likely to lead to embarrased F# devs depending on the old behavior than, say, the formatting of records. So we should get a version working which requires enough fidelity to be a good representation and not to break users, but which doesn't require completely identitcal output in all situations.
It might be possible to achieve something close to an exact mimic of existing behavior by adding extra methods, but it reduces the chance that this ever gets done, which then reduces the ability of F# developers to write performant apps. We should allow any approach that keeps reasonable output, and is unlikely to break users, as we discussed above. |
I do go back and forth on this stuff. When optimistic I say "yes, sure", when realistic I say "no, compat". The history of this goes back so far.
You can see why I sometimes a little of jaded of these "alternative .NETs" - these variations on .NET have caused 1000s of lost hours and huge opportunity costs for myself and the projects I've been associated with over the years. As an aside one of the amazing things about Mono is that it always did all of .NET (it took them several iterations for iOS though). Anyway, this seems like a viable approach to me:
That is, a single assembly-level attribute
Sound ok? |
We shouldn't rely on runtime features which are only in the specific (quite recent) version of runtime. We still need to support the full framework and netstandard versions. That said, I guess the attribute-driven approach should be the way to go. It's more explicit (we don't choose different codegen based on runtime), backwards compatible, and can be easily hidden under the compiler flag. We will probably need an RFC for it, so we have the scope of this feature documented and agreed upon. |
Support is still there. Just not new changes. |
We probably don't want different codegen based on runtime alone. |
Improved platforms and APIs, for improved codegen. Is that not desirable? |
In this case, we don't give a choice to use the simplified version if you run on "unsupported" runtime (I.e. we are locking the feature to runtime version). You will still be able to do it in the attribute-driven approach. |
What about only requiring the attribute on platforms without |
Attribute is compile-time, whereas RuntimeFeatures is, well, runtime. So we will be requiring attribute all the time, just in case we run on an unsupported runtime? |
RuntimeFeature.IsDynamicCodeSupported the property is not available compile-time? |
Not having the property is just equivalent to this returning true, and an attribute would interpret it as false. |
It is a runtime flag, yeah. Personally, I think we should go for an attribute, it's most explicit and straightforward, and will behave the same everywhere no matter what runtime. |
I would like to make progress on this. The current proposal is:
I'd like to change this to a flag |
@dsyme any other work happens in this area? I have very early attempt on hacking (nothing special) this which is stale right now, so I would like to know, if I somehow would be affected in other ways. |
I have a prototype, will share it in a bit. The following construct in FSharp.Core uses '%A' formatting: Quotation ToString/GetLayout. However Quotations in general use a lot of reflection so I think this is expected. |
Here is my work in progress. Please contribute and try it out. I want this to be largely community led if possible :) |
@charlesroddie at al - I'd really like to understand if it is only |
From my testing it’s only |
I was testing .NET 7 preview 5 SDK (daily build), which has added out of the box support for NativeAot. It seems to handle A quick How-ToOS: Ubuntu 20.04 x64 Setup: mkdir ~/.dotnet7
curl -sSL https://aka.ms/dotnet/7.0.1xx/daily/dotnet-sdk-linux-x64.tar.gz | tar xzf - -C ~/.dotnet7
alias dotnet7=~/.dotnet7/dotnet
cat > ~/NuGet.config <<EOF
<configuration>
<packageSources>
<add key="dotnet7" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet7/nuget/v3/index.json" />
</packageSources>
</configuration>
EOF Create project: dotnet7 new console -n fsharpnative1 --language f#
cd fsharpnative1 Program.fs // snippet copy & paste from https://fsharpforfunandprofit.com/posts/printf/
open System
// tuple printing
let t = (1,2)
Console.WriteLine("A tuple: {0}", t)
printfn "A tuple: %A" t
// record printing
type Person = {First:string; Last:string}
let johnDoe = {First="John"; Last="Doe"}
Console.WriteLine("A record: {0}", johnDoe )
printfn "A record: %A" johnDoe
// union types printing
type Temperature = F of int | C of int
let freezing = F 32
Console.WriteLine("A union: {0}", freezing )
printfn "A union: %A" freezing Publish: dotnet7 publish -c Release --use-current-runtime -p:PublishAot=true there are warnings/errors, but the build succeeds:
Run it:
Output:
Despite the error:
ILC succeeds. If we can work this error out first in .NET 7 timeframe, that would be cool (since it seems benign, and showing up for a simple "Hello from F#" app as well). |
Regular NativeAOT works just fine with %A but there extra goal to make it work in reflection-free mode which is not supported (but working). Having reflection obviously fine, but being not rely on it is also goal which a lot of people want. This issue is to improve reflectionfree mode too. https://github.com/kant2002/RdXmlLibrary/blob/main/FSharp.Core.xml You can plug this file with
|
The compiler detected there's a generic cycle in the assembly and instead of compiling until it runs out of memory (or the heath death of the universe, whichever comes first) cut off the generic expansion at the point when it ran over the cutoff. If the reported method is reached at runtime, it will throw. FWIW, the cycle(s) involve following entities. The compiler should print them, not sure why it's not kicking in here:
|
Just for information about other reflection-free mode limitations. Construct
|
I also receive this error with string interpolation in regular NativeAOT
|
Related, adding for prosperity: #429 |
The need for AOT and linker support
.Net code is deployed to devices where performance - including startup time - and download size are important.
JIT is ruled out by this and may be explicitly prohibited (UWP and iOS) or result in unacceptably slow application startup (Xamarin.Android).
Current .Net plans (.Net Form Factors) involve a supported AOT option across .Net, with greater usage of linkers:
F# string problems affecting AOT/linker support
F# Support lists incompatibilities with CoreRT and .Net Native. These pick out the aspects of F# that are likely to be problematic for any performant AOT and linkers. (Note: F# is generally compatible with current mono AOT but this is not performant, and compatibility may not include linking.)
The largest problem here is F# string methods:
string
,ToString()
, andsprintf %A
do.The offending part of FSharp is lacking in type safety, with everything done via casting, uses reflection without restraint, and encapsulates poorly (e.g.
.ToString()
methods in FSharp record and DU types just callingsprintf "%A"
on the entire record).Solution part 1: localize by generating ToString() overrides on F# types
We have effectively, on F# record and DU types (just check SharpLab to confirm this):
The sprintf "function" doesn't have legitimate access to the data needed to generate the string, so has to use reflection to get the structure.
Instead this method should be compiled:
Note that once this is done,
CompiledToString
does not need to know how to print records and DUs.Solution part 2: compile
A method (represented above as pseudocode
CompiledToString<'t>
) should be created to generate compiled code for any concrete type't
, to be used in place of the current dynamic sprintf code where possible.Where the method sees only
obj
it could preferably use .ToString(), or else use a very light form of reflection, making sure that codegen is not used.Solution part 3: integrate
We need to decide where to use the method
CompiledToString<'t>
:This has so much nicer ergonomics than
sprintf
that it is likely to replacesprintf
in most F# code. If string interpolation is always compiled and ToString() methods work as above, then it will be easy for F# users to avoid AOT/linker incompatibilities.sprintf
: Compilesprintf
where possible dotnet/fsharp#8621 . Note that we'd need to preserve some functionality for displaying records to deal with F# libraries compiled with earlier versions of F#: if they haveoverride t.ToString() = sprintf "%A" t
, thensprintf "%A" t
can't useToString()
.It may be simplest to start by doing this for string interpolation as the first step, adding extra methods and preserving existing sprintf code, and afterwards migrate this work to sprintf.
TBD
Generics/inlines
The text was updated successfully, but these errors were encountered: