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

Add support for asynchronous macro expansion #2896

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

roopekv
Copy link
Contributor

@roopekv roopekv commented Nov 14, 2024

This adds support for asynchronous macro expansion by declaring async overloads for assertMacroExpansion and the various expansion 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 named AsyncSyntaxRewriter marked with the @_spi(MacroExpansion) attribute for implementing an asynchronous version of MacroApplication in the SwiftSyntaxMacroExpansion 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 of AsyncSyntaxRewriter from outside of the package, but currently, it isn't supported.

Add async overloads of expansion functions to macro protocols

Adds 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 the SwiftCompilerPlugin level.

Copy link
Member

@ahoppen ahoppen left a 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.

@roopekv
Copy link
Contributor Author

roopekv commented Dec 4, 2024

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.

The synchronous version of the code and the added code generation step is only required for the synchronous version of assertMacroExpansion. If the synchronous version of assertMacroExpansion could be removed, so could all the synchronous code facilitating it, in addition to the added code generation step.

As assertMacroExpansion is mainly used in tests, and all the major testing frameworks (Swift Testing, XCTest) support asynchronous tests, the migration process would be as easy as marking the test functions as async and adding the await keyword before the assertMacroExpansion calls. As a compromise, the users could be given a heads-up by marking the synchronous assertMacroExpansion as deprecated, and removing it in a later release along with all the extra 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.

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.

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 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?

@ahoppen
Copy link
Member

ahoppen commented Dec 4, 2024

As assertMacroExpansion is mainly used in tests, and all the major testing frameworks (Swift Testing, XCTest) support asynchronous tests, the migration process would be as easy as marking the test functions as async and adding the await keyword before the assertMacroExpansion calls. As a compromise, the users could be given a heads-up by marking the synchronous assertMacroExpansion as deprecated, and removing it in a later release along with all the extra code.

I don’t think that having only an asynchronous assertMacroExpansion function is the right solution unless there is a very good reason for it. You never know in which context people might be using assertMacroExpansion (they could be calling it from a closure that doesn’t support async calls) and forcing async/await on them when there is no reason to unless their macro is using async functions (which I expect 99% of all macros to not) does not seem right to me.

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?

My understanding is that first macro proposals included an async expansion function because there was the idea that macros could call back into the compiler to retrieve additional information (such as types). As we gained more experience with macros, we realized that such facilities are not needed or wanted, so the need for async never arose. But since I’m not the author of these proposals, I’m not the right person to give the final decision here. The language steering group has accepted the revision to the macro proposals, making the expansion functions synchronous here.

@roopekv
Copy link
Contributor Author

roopekv commented Dec 4, 2024

You never know in which context people might be using assertMacroExpansion (they could be calling it from a closure that doesn’t support async calls) and forcing async/await on them when there is no reason to unless their macro is using async functions (which I expect 99% of all macros to not) does not seem right to me.

I cannot think of a valid use case for assertMacroExpansion outside of testing. After all, it's part of the TestSupport modules. I would even go as far as to say that assertMacroExpansion shouldn't be used outside of tests at all, as the purpose of macros is not to be called as regular functions. If such behavior is desired, the implementation of the macro should be separated from the macro itself, allowing its functionality to be used from regular code.

Is there a case where a macro expansion test couldn't be asynchronous?

My understanding is that first macro proposals included an async expansion function because there was the idea that macros could call back into the compiler to retrieve additional information (such as types). As we gained more experience with macros, we realized that such facilities are not needed or wanted, so the need for async never arose. But since I’m not the author of these proposals, I’m not the right person to give the final decision here. The language steering group has accepted the revision to the macro proposals, making the expansion functions synchronous swiftlang/swift-evolution#2546.

Thanks for the clarification. There is an announcement about the revision on the Swift Forums that says:

In line with our "shrink-to-fit" process, we are incorporating edits to SE-0382 and SE-0389 that reflect the API as implemented. Notably, this does not necessarily preclude "async-ified" APIs being included at some point in the future, but it would not be as part of these proposals.

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?

@ahoppen
Copy link
Member

ahoppen commented Dec 5, 2024

Is there a case where a macro expansion test couldn't be asynchronous?

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 SyntaxRewriter and an AsyncSyntaxRewriter. And really, if we want to support asynchronous macros, we should have an AsyncSyntaxVisitor, AsyncSyntaxAnyVisitor, an asynchronous version of ancestorOrSelf, etc. And all of these types need to continue to have a synchronous version because many, if not most applications outside of macros will be synchronous.

Does this mean that an additional Swift Evolution proposal would be required for asynchronous expansion to be included in swift-syntax?

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.

@roopekv
Copy link
Contributor Author

roopekv commented Dec 5, 2024

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 may have found a significant real-world benefit for all users of macros that could be made possible by asynchronous expansion: processing macro invocations in parallel within a single process, instead of launching each of them as separate processes. EDIT: This could also be done without asynchronous expansion functions, as pointed out in later comments.

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?

@ahoppen
Copy link
Member

ahoppen commented Dec 5, 2024

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.

@roopekv
Copy link
Contributor Author

roopekv commented Dec 5, 2024

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.

@roopekv
Copy link
Contributor Author

roopekv commented Dec 5, 2024

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?

@ahoppen
Copy link
Member

ahoppen commented Dec 5, 2024

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 asynchronous to facilitate parallelism at the plugin server level after all I guess?

Exactly.

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

Successfully merging this pull request may close these issues.

2 participants