search

fsprojects / fantomas

FSharp source code formatter
https://fsprojects.github.io/fantomas
Other
758 stars 189 forks source link

Make Fantomas AOT compatible #3084

Open TobyShaw opened 1 month ago

TobyShaw commented 1 month ago

Using fantomas to format a single file via the command line is very slow, ~700-1000ms depending on the size of the file.

I hypothesised that this was due to the JIT taking time to warm up (not an issue with fantomas --daemon or formatting many files). In a test, I got fantomas to compile with AOT enabled, and was able to reduce the time to ~70-100ms, a legitimate 10x speedup.

The changes required are:

It's not obvious how an AOT executable is distributed as a dotnet tool, I'd be happy enough if building/packaging/deploying the AOT fantomas was left as an exercise for power users.

Would any of the changes I suggested be accepted? If we do need to fork fantomas for this change, ideally we keep our diff as small as possible.

Please tick all that apply:

baronfel commented 1 month ago

Currently dotnet tools cannot be distributed as AOT applications - fantomas would need another solution for downloading/acquisition. One hurdle to producing and distributing AOT applications of all kinds is that you need to publish the application separately on each architecture that you want to have an AOT app for (this is a limitation of NativeAOT tech today), then after all of that compilation create a meta-artifact of some kind. It's not easy by any means today.

I logged https://github.com/dotnet/sdk/issues/40931 to track this in the SDK code base.

nojaf commented 1 month ago

Hi Toby! I think I'm on board with this idea. It would indeed be a power-user thing probably.

What architectures would you be using? Thinking out loud, could we do the "compile to single file thing" and have it as part of our GitHub release artefacts? I think those produce a download link which we could use for distribution.

TobyShaw commented 1 month ago

GitHub release artefacts sounds like a good idea given the difficulty of bundling it in the dotnet tool. In my test I produced a single exe/pdb, so I think this approach is viable.

We'd only need it for linux-x64 right now.

nojaf commented 1 month ago

If we create the file in our build script (build.fsx) and just append it to the other files here

https://github.com/fsprojects/fantomas/blob/873d9d70d755f2c058f49b4712f3d22abb28dce4/build.fsx#L449-L466

that probably will do the right thing.

@josh-degraw, @dawedawe any concerns here? You guys on board with this?

TobyShaw commented 1 month ago

I'll highlight the most contentious aspect of this change is that we'd need to rip out Argu entirely. I figured there must be a non-reflection-based API but there isn't.

I'd hope we can preserve command-line backwards compatibility, but it's possible Argu is idiosyncratic in some way that is hard to replicate in other libraries. My understanding is that the CLI options are very simple so this probably isn't a concern, but figured I'd raise it.

nojaf commented 1 month ago

Yeah, I'm ok with ripping out Argu. Some work will of course need to be done to keep parity. I'm really unfamiliar with the AOT rules. So did you run some sort of analysis tool that mentioned Argu is the only culprit? Or could there be other things as well?

dawedawe commented 1 month ago

I can't say I have much experience with anything .NET AOT related stuff. So I'm on board with this just for learning something. If the argu topic can't be solved in a smooth way, maybe some compiler directives are enough to get going for some time...

TobyShaw commented 1 month ago

I took a cavalier approach in my test, just compiling/running it until I got output that I expected.

The proper approach would be to aim for a zero-AOT-warning build, in principle this should flag up cases like Argu/printf. https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot/fixing-warnings

I saw some other warnings but I don't have them to hand right now, we may have to silence some false positives.

josh-degraw commented 1 month ago

I also have no experience with aot but I like the idea of 10x speedup

Numpsy commented 1 month ago

fwiw, I've recently been making some attempts at getting an F# tool at work to build both as a .NET global tool and as a Native AOT build using the same source and that broadly works, though there are many build warnings from FSharp.Core itself. This is using Fargo for command line parsing (one of the reasons I went for Fargo over Argu is listed support for AOT as I didn't know if the usage of quotations in Argu would work for AOT)

I also did some previous prep/testing work with that using self-contained/trimmed builds to shake out some issues there and to get a single file build going - I don't know if a build of Fantomas using ReadyToRun would have any perf benefits as an intermediate step?

TobyShaw commented 1 month ago

I was not aware of that flag, so I tried it out. In the same test case, ReadyToRun changed execution time down to 400ms. A significant improvement but still 4x slower than the full AOT publishing. If it's significantly easier to implement then maybe worth it as an intermediate?

TobyShaw commented 1 month ago

I've started on something in https://github.com/fsprojects/fantomas/pull/3090 Will update here if there are major developments.