[Draft] Clean build: Only integrate module dependencies once #742
Conversation
CodaFi
left a comment
CodaFi
left a comment
There was a problem hiding this comment.
Makes sense to me. Thanks!
|
@swift-ci test |
|
Please don't merge this yet. I'm in the midst of analysis and I suspect it may be introducing a bug when incremental imports is disabled. Hope to know for sure very soon! |
davidungar
left a comment
davidungar
left a comment
There was a problem hiding this comment.
See my comments. If I'm right, this fix is unsound, but it's tricky!
| // This will result in dependencies being processed for each input (because it will be considered always changed). | ||
| // We can skip, because a previous input has already integrated this dependency, | ||
| // which is why it's in `fingerprintedExternalDependencies`. | ||
| return nil |
There was a problem hiding this comment.
I think there may be a problem here: What if the external dependency has changed? It would need to be reintegrated. If the external dependency was in a priors, ifKnown will be true, so you don't get here. But, what if an external dependency in the priors has changed, it is integrated, and it includes a changed external dependency that is already in fingerprintedExternalDependencies? isKnown will not be true and I think you get into trouble here.
I believe there is a problem: isKnown really means "is guaranteed to be in fingerprintedExternalDependencies because it was found directly in the priors. But !isKnown only means it was found in a swiftdeps or swiftmodule file.
I have an idea for what I hope will be a much better fix. Let me see if it looks good. I should have the PR posted in a day.
There was a problem hiding this comment.
Yeah, I’m thinking maybe just a separate set that we can use to guarantee that dependencies are only read once per driver invocation.
Alternatively we could store mod time with the dependencies we know about, so we could compare that, instead of comparing to build start time.
Does the fingerprint change when the file contents change? Could that be used? Or is that something else?
There was a problem hiding this comment.
Please see #750 , which is my proposal to fix this issue. I would love to get your thoughts on it.
|
Closing in favor of #750 |
Not sure if this change is safe, or if it needs to go further to impact incremental builds as well.
Currently this is shaving close to 2.5 minutes off of a clean build for a project with >150 modules and ~350 swift files in the main target.
Issue before was this was reading and integrating the module dependency information (M) for each input swift file (N), so (N*M), now it's only happening M times.
It needs more testing, but there is probably an incremental build situation where dependency information is being read and integrated N * M times where M is modules that have changed since last build.