Refactor Clang runtimes building into async builder #6380
Conversation
Previously, the Clang runtimes building only considered building the target resource directory, and was only _internally_ asynchronous. Because the asynchrony was only internal, it could use the function frame as a context object throughout the build of the resource dir. This is simple but doesn't generalize well to more runtimes: if we want to add 2 or 3 more runtimes, we want them to _all_ build asynchronously. That means using some asynchronous builder that maintains the context and allows them to proceed concurrently with other work. This also factors all the runtimes building code into a separate set of files. These aren't separate libraries at this point due to the `ClangRunner` in some cases wanting to build runtimes on-demand, but it at least lets us organize the code more cleanly. Because this splits code between `clang_runner.*` and `clang_runtimes.*`, it also works to update the `#include`s for both to be roughly accurate. I used ClangD's include cleaner for this and it probably also did some latent cleaning as it went, but that's the reason for the churn of `#include` lines. The archive building is also factored out into a re-usable helper. This is a bit "over factored" in this PR, but supports the next PR that uses the same code to build archives for other runtimes. This also overhauls the synchronization used -- it uses a simple `Latch` construct introduced in a previous PR to coordinate between the steps of building the runtimes. Last but not least, it factors the "enable leaking" state out of a boolean in the runner to a parameter. This is important in the face of concurrent calls as otherwise toggling this boolean can create a race. The next PR will layer building more runtimes on top of this new factoring.
chandlerc
force-pushed
the
push-uxkynytsypqq
branch
from
b4663f9 to
8aa80b0
Compare
| llvm::fmt_consume(obj_result.takeError()))); | ||
| } | ||
|
|
||
| // Unlink the object file once we've read it. However, we log and ignore |
There was a problem hiding this comment.
Maybe say why we're unlinking the file?
| } | ||
| } | ||
|
|
||
| result_ = (*std::move(runtimes_builder_)).Commit(); |
There was a problem hiding this comment.
| result_ = (*std::move(runtimes_builder_)).Commit(); | |
| result_ = std::move(runtimes_builder_)->Commit(); |
There was a problem hiding this comment.
weirdly, I don't think this works. Because Commit needs its LHS to be R-value qualified, and you can't do that with the overloaded operator-> (which has to return a pointer), but you can with operator* (which can return an R-value ref or by-value)... =[
toolchain/driver/clang_runtimes.cpp
Outdated
| return Success(); | ||
| } | ||
|
|
||
| std::scoped_lock lock{obj_dirs_mu_}; |
There was a problem hiding this comment.
Is there some convention for () v {} in ctor init in Carbon? Usually I expect to see () for ctor calls (or = x if the ctor's implicit).
There was a problem hiding this comment.
The usual Google style guide conventions -- for some reason I thought this hit the vexing parse and needed {}s, but it doesn't. Switched back to ()s.
Co-authored-by: David Blaikie <[email protected]>
chandlerc
left a comment
chandlerc
left a comment
There was a problem hiding this comment.
Thanks for the review! I think all addressed so merging. Lemme know if you'd like any follow-ups.
| } | ||
| } | ||
|
|
||
| result_ = (*std::move(runtimes_builder_)).Commit(); |
There was a problem hiding this comment.
weirdly, I don't think this works. Because Commit needs its LHS to be R-value qualified, and you can't do that with the overloaded operator-> (which has to return a pointer), but you can with operator* (which can return an R-value ref or by-value)... =[
toolchain/driver/clang_runtimes.cpp
Outdated
| return Success(); | ||
| } | ||
|
|
||
| std::scoped_lock lock{obj_dirs_mu_}; |
There was a problem hiding this comment.
The usual Google style guide conventions -- for some reason I thought this hit the vexing parse and needed {}s, but it doesn't. Switched back to ()s.
| llvm::fmt_consume(obj_result.takeError()))); | ||
| } | ||
|
|
||
| // Unlink the object file once we've read it. However, we log and ignore |
77808cd
The review of carbon-language#6380 suggested an expanded comment that I wrote but apparently didn't hit "save" in the editor for. Doh! This adds it.
The review of #6380 suggested an expanded comment that I wrote but apparently didn't hit "save" in the editor for. Doh! This adds it.
3 participants
Previously, the Clang runtimes building only considered building the target resource directory, and was only internally asynchronous. Because the asynchrony was only internal, it could use the function frame as a context object throughout the build of the resource dir. This is simple but doesn't generalize well to more runtimes: if we want to add 2 or 3 more runtimes, we want them to all build asynchronously. That means using some asynchronous builder that maintains the context and allows them to proceed concurrently with other work.
This also factors all the runtimes building code into a separate set of files. These aren't separate libraries at this point due to the
ClangRunnerin some cases wanting to build runtimes on-demand, but it at least lets us organize the code more cleanly.Because this splits code between
clang_runner.*andclang_runtimes.*, it also works to update the#includes for both to be roughly accurate. I used ClangD's include cleaner for this and it probably also did some latent cleaning as it went, but that's the reason for the churn of#includelines.The archive building is also factored out into a re-usable helper. This is a bit "over factored" in this PR, but supports the next PR that uses the same code to build archives for other runtimes.
This also overhauls the synchronization used -- it uses a simple
Latchconstruct introduced in a previous PR to coordinate between the steps of building the runtimes.Last but not least, it factors the "enable leaking" state out of a boolean in the runner to a parameter. This is important in the face of concurrent calls as otherwise toggling this boolean can create a race.
The next PR will layer building more runtimes on top of this new factoring.