[Dependency Scanning] Refactor the scanner to simplify layering #82429
+568
−651
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
force-pushed
the
ScannerInvalidBinaryModuleRefactor
branch
from
0723261 to
c094bb7
Compare
- 'SwiftModuleScanner' will now be owned directly by the 'ModuleDependencyScanningWorker' and will contain all the necessary custom logic, instead of being instantiated by the module interface loader for each query - Moves ownership over module output path and sdk module output path directly into the scanning worker, instead of the cache
…a reference to the global 'SwiftDependencyScanningService' While this made sense in the distant past where the scanning service provided backing storage for the dependency cache, it no longer does so and now makes for awkard layering where clients get at the service via the cache. Now the cache is a simple data structure while all the clients that need access to the scanning service will get it explicitly.
artemcm
force-pushed
the
ScannerInvalidBinaryModuleRefactor
branch
from
c094bb7 to
39c096c
Compare
|
@swift-ci test |
artemcm
requested review from
xymus,
hborla,
slavapestov,
xedin,
cachemeifyoucan,
zoecarver,
egorzhdan,
beccadax,
ian-twilightcoder,
Xazax-hun,
j-hui,
fahadnayyar and
susmonteiro
as code owners
| @@ -992,7 +992,7 @@ class SwiftDependencyScanningService { | |||
| std::shared_ptr<llvm::cas::ActionCache> ActionCache; | |||
|
|
|||
| /// File prefix mapper. | |||
| std::unique_ptr<llvm::PrefixMapper> Mapper; | |||
| std::shared_ptr<llvm::PrefixMapper> Mapper; | |||
There was a problem hiding this comment.
Can this be moved to worker? I feel like this should be shared, probably only here due to some unfortunate layering too.
There was a problem hiding this comment.
The reason I didn't move it to the worker here is because ExplicitModuleDependencyResolver uses the prefix mapper and doesn't have access to the ModuleDependencyScanner or its associated workers, unfortunately.
There was a problem hiding this comment.
Resolver holds an instance. We can totally rebuild the map from a CompilerInstance and we should probably do that to fully decouple the layering.
There was a problem hiding this comment.
We can maybe even use the CAS from the instance, if that is possible.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Refactor
ModuleDependenciesCacheto not hold a reference to the globalSwiftDependencyScanningServiceWhile this made sense in the distant past where the scanning service provided backing storage for the dependency cache, it no longer does so and now makes for awkward layering where clients get at the service via the cache. Now the cache is a simple data structure while all the clients that need access to the scanning service will get it explicitly.
Refactor Swift Scanner loader to be standalone
SwiftModuleScannerwill now be owned directly by theModuleDependencyScanningWorkerand will contain all the necessary custom module query logic, instead of being instantiated by the module interface loader for each query.ModuleLoaderno longer needs to have virtual API for this functionality.Move ownership over module output path and sdk module output path directly into the scanning worker, instead of the cache.