-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[Doc] Update documentation for no-transitive-change #96453
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
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This may have been discussed somewhere that I missed, please point me to that if that's the case.
I was putting a little more thought into it and am wondering what would be a way for the build systems to take advantage of that?
When compiling something that dependends on modules, the build system will need to check if any of the (transitive) dependencies of the modules changes, right? Because any BMI that was changed in the transitive set can potentially affect the result of the current compilations.
So even if direct dependencies did not change (because of this optimization), despite a change in the deeper transitive dependency, the build system still can't reuse the result and has to rerun the compile.
We only know that the output BMI wasn't affected by any of the transitive changes after we finish compiling it and can compare the outputs, right?
But the build system needs to know about it before the compilation happens to avoid recompilations.
I suspect I'm missing something, but don't know what...
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.
Yes, and this is the reason why it is experimental.
It is not about to avoid the re-compilation for that unchanged BMI, but for other TUs that only dependent on the unchanged BMI. For example,
For this example, every time
a.cppm
changes, we need to recompileb.cppm
. But if the BMI of moduleB
doesn't change at all, it should be good to not recompilec.cpp
.So this is the ability provided by the compiler that all the needed changes are propagated to the BMI:
llvm-project/clang/lib/Serialization/ASTWriter.cpp
Lines 1229 to 1230 in c791d86
The theory of this is that, the users of a build system, can only access the entities in the indirectly imported modules via the directly imported modules. So that the directly imported modules have a full control of accessiable entities in the indirectly imported modules for their (the directly imported modules) users.
Maybe it is a good idea for the build system to provide a verify option that the skippable compilations can have the same result if they are not skipped.
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.
That's what I thought is happening, thanks for confirming my intuition.
I think it'd be great to spell this out in the documentation, because I believe this mode of operation is something that the build system vendors might need to infer from the whole text right now.
Adding something like the following should do the trick:
PS My 2 cents on Bazel: I suspect this actually goes against its design. Bazel has very strong capabilities to avoid recompilations when inputs don't change, but it relies on stable hashes for all inputs (including transitive dependencies).
At the same time I suspect that other build systems (
CMake
+ccache
/sccache
) are more flexible in that regard.PPS I am not a build system expert, so take that with a grain of salt. I'm also happy to loop in Bazel folks I work with to confirm or rebut my claims about Bazel's design, if that's useful.
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.
Yeah, added suggested text.
For bazel, I think it might be fine since it should be configurable that whether not to recompile a target as far as I know.