-
-
Notifications
You must be signed in to change notification settings - Fork 193
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
Comments
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. |
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. |
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. |
If we create the file in our build script ( Lines 449 to 466 in 873d9d7
that probably will do the right thing. @josh-degraw, @dawedawe any concerns here? You guys on board with this? |
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. |
Yeah, I'm ok with ripping out Argu. Some work will of course need to be done to keep parity. |
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. |
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. I saw some other warnings but I don't have them to hand right now, we may have to silence some false positives. |
I also have no experience with aot but I like the idea of 10x speedup |
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. 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? |
I was not aware of that flag, so I tried it out. |
I've started on something in #3090 |
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:
The text was updated successfully, but these errors were encountered: