Skip to content
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 Fantomas AOT compatible #3084

Open
1 of 4 tasks
TobyShaw opened this issue May 16, 2024 · 12 comments
Open
1 of 4 tasks

Make Fantomas AOT compatible #3084

TobyShaw opened this issue May 16, 2024 · 12 comments

Comments

@TobyShaw
Copy link

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:

  • Upgrade to net8.0
  • Remove Argu (or swap to a non-Quotation-based interface)
  • Remove $"%A{..}" style string building in ASTTransformer
  • Sprinkle some msbuild properties

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:

  • This is not a breaking change to Fantomas
  • I or my company would be willing to help implement and/or test this
  • This suggestion is part of the Microsoft style guide (please add a link to section if so)
  • This suggestion is part of the G-Research style guide (please add a link to section if so)
@baronfel
Copy link
Contributor

baronfel commented May 16, 2024

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 dotnet/sdk#40931 to track this in the SDK code base.

@nojaf
Copy link
Contributor

nojaf commented May 17, 2024

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
Copy link
Author

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
Copy link
Contributor

nojaf commented May 17, 2024

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

fantomas/build.fsx

Lines 449 to 466 in 873d9d7

let files = nugetPackages |> String.concat " "
// We create a draft release for minor and majors. Those that requires a manual publish.
// This is to allow us to add additional release notes when it makes sense.
let! draftResult =
let isDraft =
let isRevision = lastRelease.Version.Split('.') |> Array.last |> int |> (<>) 0
if isRevision then String.Empty else "--draft"
Cli
.Wrap("gh")
.WithArguments(
$"release create v{currentRelease.Version} {files} {isDraft} --title \"{currentRelease.Title}\" --notes-file \"{noteFile}\""
)
.WithValidation(CommandResultValidation.None)
.ExecuteAsync()
.Task
|> Async.AwaitTask

that probably will do the right thing.

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

@TobyShaw
Copy link
Author

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
Copy link
Contributor

nojaf commented May 17, 2024

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
Copy link
Member

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
Copy link
Author

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
Copy link
Contributor

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

@Numpsy
Copy link

Numpsy commented May 20, 2024

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
Copy link
Author

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
Copy link
Author

I've started on something in #3090
Will update here if there are major developments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants