[Dependency Scanning] Refactor batch clang dependency query to preserve only partial results #84929
+306
−169
Conversation
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
artemcm
requested review from
cachemeifyoucan,
hborla,
slavapestov and
xedin
as code owners
|
@swift-ci test |
| struct BatchClangModuleLookupResult { | ||
| llvm::StringMap<clang::tooling::dependencies::ModuleDeps> | ||
| discoveredDependencyInfos; | ||
| llvm::StringMap<std::vector<std::string>> visibleModules; |
There was a problem hiding this comment.
StringMap is not thread-safe. A normal insert might be but it can expand when full. We need a lock for this.
There was a problem hiding this comment.
We do have a lock for this, it is only ever modified in performParallelClangModuleLookup in a critical section guarded with a lock.
There was a problem hiding this comment.
I will add a helper function to the class to ensure the correct lock usage. It might even be cleaner to move the lock here to avoid introduce regression for future refactoring.
| struct BatchClangModuleLookupResult { | ||
| llvm::StringMap<clang::tooling::dependencies::ModuleDeps> | ||
| discoveredDependencyInfos; | ||
| llvm::StringMap<std::vector<std::string>> visibleModules; |
There was a problem hiding this comment.
I will add a helper function to the class to ensure the correct lock usage. It might even be cleaner to move the lock here to avoid introduce regression for future refactoring.
…ve only partial results
Previously, with the change to bridge Clang dependency scanning results on-demand, the scanner would execute Clang dependency scanning queries for each unresolved import, in parallel, and aggregate all of the results to post-process (including on-demand bridging) later. As a consequence of that change, all of the Clang scanner queries' results ('TranslationUnitDeps') got aggregated during a scan and had their lifetimes extended until a later point when they got processed and added to the scanner's cache.
This change refactors the Clang dependency scanner invocation to, upon query completion, accumulate only the 'ModuleDeps' nodes which have not been registered by a prior scan, discarding the rest of the 'TranslationUnitDeps' graph. The arrgegated 'ModuleDeps' objects are still bridged on-demand downstream.
This change further splits up the 'resolveAllClangModuleDependencies' method's functionality to improve readability and maintainability, into:
- 'gatherUnresolvedImports' method which collects all of collected Swift dependents' imports which did not get resolved to Swift dependencies
- 'performParallelClangModuleLookup' which actually executes the parallel queries and includes the new logic described above
- 'cacheComputedClangModuleLookupResults' method which takes the result of the parallel Clang scanner query and records in in the Swift scanner cache
- 'reQueryMissedModulesFromCache' method which covers the scenario where Clang scanner query returned no result because either the dependency can only be found transitively, or the query is for a dependency previously-queried.
artemcm
force-pushed
the
RevertRevertOnDemandBridging
branch
from
98fb1ff to
8335a03
Compare
|
@swift-ci smoke test |
|
@swift-ci smoke test Windows platform |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Previously, with the change to bridge Clang dependency scanning results on-demand, the scanner would execute Clang dependency scanning queries for each unresolved import, in parallel, and aggregate all of the results to post-process (including on-demand bridging) later. As a consequence of that change, all of the Clang scanner queries' results (
TranslationUnitDeps) got aggregated during a scan and had their lifetimes extended until a later point when they got processed and added to the scanner's cache.This change refactors the Clang dependency scanner invocation to, upon query completion, accumulate only the
ModuleDepsnodes which have not been registered by a prior scan, discarding the rest of theTranslationUnitDepsgraph. The arrgegatedModuleDepsobjects are still bridged on-demand downstream.This change further splits up the
resolveAllClangModuleDependenciesmethod's functionality to improve readability and maintainability, into:gatherUnresolvedImportsmethod which collects all of collected Swift dependents' imports which did not get resolved to Swift dependenciesperformParallelClangModuleLookupwhich actually executes the parallel queries and includes the new logic described abovecacheComputedClangModuleLookupResultsmethod which takes the result of the parallel Clang scanner query and records in in the Swift scanner cachereQueryMissedModulesFromCachemethod which covers the scenario where Clang scanner query returned no result because either the dependency can only be found transitively, or the query is for a dependency previously-queried.