-
Notifications
You must be signed in to change notification settings - Fork 416
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
Add support for asynchronous macro expansion #2896
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR, @roopekv.
This PR adds a lot of code, which has cost associated with it, such as an increased binary size, higher maintainance cost because there are an async and a sync version of a function that need to be kept in sync, and because updating generated code is harder than non-generated code. At the same time, only a subset of swift-syntax works well with async functions (for example there is no async visitor). So, you could argue that this PR is too large and too small at the same time.
Furthermore, given this non-trivial cost, this change needs to provide some significant, real-world benefits and I don’t see those right now: The two scenarios in which Swift Concurrency provides the biggest benefits are (1) if the program needs to halt to wait for network requests, file access etc. and (2) to execute expensive work across multiple cores. (1) does not apply to macros because they should not read data from the outside world. Regarding (2), macros should not perform so much work that they benefit from multi-threading. They are only given a very limited subset of the source file and processing that should not take so much time that it makes sense to go through the overhead of spinning up mulitple cores. Multi-threading might even be harmful because the build system will schedule compiler invocations in parallel to use all CPU cores. If one of these invocations uses multiple cores, it can lead to machine over-subscription.
The synchronous version of the code and the added code generation step is only required for the synchronous version of As
The scope of this PR can be expanded to other parts of the package if required, or those issues could be tackled in separate ones.
The accepted Swift Evolution proposals for macros stated that the macro expansion operation can be asynchronous before I pointed out that the implementation in swift-syntax didn't match those proposals in #2803. Perhaps the potential benefits and drawbacks should be discussed with the wider Swift community before completely ruling out the idea? |
I don’t think that having only an asynchronous
My understanding is that first macro proposals included an |
I cannot think of a valid use case for Is there a case where a macro expansion test couldn't be asynchronous?
Thanks for the clarification. There is an announcement about the revision on the Swift Forums that says:
https://forums.swift.org/t/update-on-se-0382-and-se-0389-expression-macros-and-attached-macros/74094 Does this mean that an additional Swift Evolution proposal would be required for asynchronous expansion to be included in swift-syntax? |
A spontaneous idea that I would have right now would be that somebody could write ["input1", "input2"].forEach {
assertMacroExpansion($0, ...)
} This is probably not the biggest blocker but it is worth considering. I am more concerned about having to maintain a
I am not a member of the Language Steering Group, so I can’t make the final decision here but I don’t think it would require an evolution proposal. But since this is a pretty big change, we should have a discussion about this in the Macros category of the Swift Forums. But as I said in my initial comment, I think we would need to see some significant real-world benefits from the switch to asynchronous macros in order to take this change. |
I found a few threads on the Swift Forums lamenting the performance of Macros, mostly about the compile time of the macros themselves, but also concerning the macro invocation/expansion phase of the compilation (example). I believe that the introduction of asynchronous expansion could allow for a faster, data race safe, and possibly a more parallel expansion of macros, with the opportunity for significant compile time reduction for all macros, both synchronous and asynchronous. The act of launching processes in itself has overhead, so not having to launch more than one process per plugin should be a performance win regardless of whether parallelism is utilized. And while multiple processes can be safely run in parallel too, that would occur with redundant memory usage compared to running multiple macro invocations in a single process on multiple threads. Asynchronous macro expansion via the Swift Concurrency model also guarantees data race safety and forward progress, making it safer than running macro invocations in parallel with other in-process methods like GCD. Macros would still run sandboxed, isolated to the plugin which they are defined in with no way to tamper with the memory of other processes. Consecutive and parallel macro invocations could affect each other if they explicitly modify and read shared memory within the plugin, but this could be considered a positive as it could allow for some clever caching behavior for advanced users. The concept could be taken even further by linking macro plugins directly to the compiler as dynamic libraries, eliminating interprocess communication entirely, but it might not be feasible due to security concerns compared to running them in separate sandboxed processes. What do you think, would this make any sense? |
I think there is a disconnect here about where parallelism is introduced. I agree that parallelism speeds up compilation, but I don’t think that the macro expansion function is the right place for parallelism. As I mentioned before, the build system already introduces parallelism by launching multiple compiler processes to utilize multiple CPU cores even if each compiler process only runs on a single core. If we wanted to (and I’m not saying here that we should), the compiler could also expand multiple macros in parallel during compilation, while each expansion function is sequential. Regarding your comment on process launching: If I remember correctly, each compiler frontend process only spawns one plugin server for each macro and uses that to perform all expansions for that macro, so it’s not like we launch a new plugin server for each expansion. |
Yes, utilizing parallelism within the macro expansion functions would only be beneficial in certain cases, which we have already discussed. But in my previous comment I was talking about parallelism in relation to processing multiple macro invocations within a single macro plugin process, without the overhead of launching multiple processes or being limited to running them sequentially. |
But if the synchronous macro expansion calls can be wrapped and run in asynchronous tasks within the process, then the macro expansion wouldn't have to be asynchronous to facilitate parallelism at the plugin server level after all, I guess? |
Exactly. |
This adds support for asynchronous macro expansion by declaring async overloads for
assertMacroExpansion
and the variousexpansion
functions discussed with @ahoppen at #2803, making it possible to utilize Swift's concurrency features in macros.Add
AsyncSyntaxRewriter
Adds an asynchronous version of
SyntaxRewriter
namedAsyncSyntaxRewriter
marked with the@_spi(MacroExpansion)
attribute for implementing an asynchronous version ofMacroApplication
in theSwiftSyntaxMacroExpansion
module.If the
package
access level supported subclassing and overriding of functions from other modules, it could be used instead of@_spi(...) open
to restrict the use ofAsyncSyntaxRewriter
from outside of the package, but currently, it isn't supported.Add async overloads of
expansion
functions to macro protocolsAdds asynchronous overloads for
expansion
functions of the various macro protocols with default implementations that call the synchronous versions.Add support for async macro expansion
Adds asynchronous overloads for
assertMacroExpansion
functions and support for asynchronous macro expansion down to theSwiftCompilerPlugin
level.