[Dependency Scanning] Update Swift Interface Module's Output Path after `vfs` Pruning #79297

qiongsiwu · 2025-02-11T18:52:16Z

This PR teaches the Swift dependency scanner to recompute Swift module interface hashes after vfs overlay pruning.

The swift dependency scanner is already aware of unused vfs overlays, and removes them from the command line. However, it did not recompute the hashes in the module's full path. The hash still took the unused vsf overlay options into account, and led to redundant variants.

This PR implements recalculation of the module output path after the vfs pruning, and uses an updated list of commands to compute the module output paths. The command line output is refreshed with the new output path, and the dependency information is updated with the new module output path and the new hash.

rdar://144256093

qiongsiwu

Recording some existing discussions with @artemcm .

include/swift/AST/ModuleDependencies.h

lib/Frontend/ModuleInterfaceLoader.cpp

cachemeifyoucan · 2025-02-12T01:07:27Z

Can you also briefly explain what problem does it solve? Is it generating multiple swift interface compilation job for different target even though they can be de-duplicated?

qiongsiwu · 2025-02-12T03:10:33Z

Can you also briefly explain what problem does it solve? Is it generating multiple swift interface compilation job for different target even though they can be de-duplicated?

Yes! If by target we mean different projects or compilation configuration. I assume we did not mean the target triple. This is solving the problem where we did not re-compute interface variants after we prune the unused vfs options. Although we removed the vfs options from the command line, the output module's name still contains a hash that considers the unused vfs options. This results in multiple module variants compiled assuming the vfs options are not unused.

cachemeifyoucan · 2025-02-12T14:47:52Z

Yes! If by target we mean different projects or compilation configuration.

Yes, I mean build target. Thanks for explanation. The scanning is happening on swift module/build target level for swift so the extra variant is only happening when you need to plan more than one build target.

lib/Serialization/ScanningLoaders.cpp

qiongsiwu · 2025-02-12T23:17:04Z

@swift-ci Please smoke test

…s pass, and the unused vfs overlay is optimized.

…ew code.

…ut paths consistently and avoid const_casts.

…d to manage any states.

qiongsiwu · 2025-02-13T04:21:12Z

@swift-ci Please smoke test

qiongsiwu · 2025-02-13T16:49:04Z

It is not clear to me why the Linux and Windows builds are failing. I rebased the branch but the same failure still occurred.

The errors do not seem to related to this PR?

Linux build error:

/home/build-user/swift-docc-symbolkit/Sources/SymbolKit/SymbolGraph/Relationship/Relationship.swift:196:47: error: inout argument could be set to a value with a type other than '[CodingUserInfoKey : any Sendable]'; use a value declared as type '[CodingUserInfoKey : Any]' instead

Windows build error:

C:\Users\swift-ci\jenkins\workspace\swift-PR-windows\sourcekit-lsp\Sources\LanguageServerProtocolJSONRPC\JSONRPCConnection.swift:370:50: error: type '(RequestID) -> Optional<any ResponseType.Type>' does not conform to the 'Sendable' protocol

Or maybe this PR is causing issues for the Sendable protocol somehow on Windows/Linux? Any input is appreciated!

artemcm · 2025-02-13T17:25:07Z

@swift-ci smoke test

cachemeifyoucan

Can you also try add a test when dependency scanning using -direct-clang-cc1-module-build option? That is the option used by caching build and I would like to shift to the new mode when time is appropriate.

The difference is that using that mode, -Xcc flag will be passing clang cc1 argument during interface compilation and the cc1 flag should be optimized by clang dependency scanner already.

cachemeifyoucan · 2025-02-13T17:48:16Z

lib/DependencyScan/ScanDependencies.cpp

+
+    SwiftInterfaceModuleOutputPathResolution::ArgListTy extraArgsList;
+    const SwiftInterfaceModuleOutputPathResolution::ArgListTy
+        &clangImpporterOptions =


cachemeifyoucan · 2025-02-13T17:54:03Z

include/swift/Frontend/ModuleInterfaceLoader.h

+
+using ArgListTy = std::vector<std::string>;
+
+void getOutputPath(ResultTy &outputPath, const StringRef &moduleName,


Nit: it is weird for getOutputPath function is returning void. Should it just return ResultTy or rename the function to something like updateHashAndOutputPath

cachemeifyoucan · 2025-02-13T17:57:58Z

lib/DependencyScan/ScanDependencies.cpp

+    SwiftInterfaceModuleOutputPathResolution::ResultTy swiftInterfaceOutputPath;
+    if (resolvingDepInfo.isSwiftInterfaceModule()) {
+      pruneUnusedVFSOverlay(swiftInterfaceOutputPath);
+      updateSwiftInterfaceModuleOutputPath(swiftInterfaceOutputPath);


I don't feel you want to make to two separate function call here, especially when nothing is pruned, there is no need to go through and update everything.

The rough break down for how the updating process works is finalize() will update the module info to avoid repeated updating for the command-line for minor changes since that is not efficient. Everything before that is information collecting to help the finalize function to do the final update, and Resolver class itself has private fields to hold the information collected.

I don't feel you want to make to two separate function call here, especially when nothing is pruned, there is no need to go through and update everything.

I can fold updateSwiftInterfaceModuleOutputPath into pruneUnusedVFSOverlay to avoid two function calls. I am a bit confused by the comment when nothing is pruned, there is no need to go through and update everything. I think in the case of swift interface modules, we no longer set its output path and context hash until here (relevant discussion). My understanding is that we want to update the command line only after we compute the correct output path, whether we have some options to prune or not. This way we don't end up with a path that can potentially points to nothing. Could you help me understand in which cases I can avoid the update?

I might be confused a little bit when I review the code because I thought the function is adding the output path to the command-line. I am a bit confused who is in charged of updating the -o flag on the command-line for building interface.

It seems that this is where we compute the output path and add it to the command line.

swift/lib/Serialization/ScanningLoaders.cpp

Line 215 in 8c8d573

Args.push_back(outputPathBase.str().str());

I think after this PR lands, I shall refactor this code. I don't think we need to calculate -o command here. We can directly calculate the output path when we are in the resolver.

Posted #79752 to refactor how we add -o to the command line.

qiongsiwu · 2025-02-13T20:53:27Z

Can you also try add a test when dependency scanning using -direct-clang-cc1-module-build option? That is the option used by caching build and I would like to shift to the new mode when time is appropriate.

The difference is that using that mode, -Xcc flag will be passing clang cc1 argument during interface compilation and the cc1 flag should be optimized by clang dependency scanner already.

Should I use -experimental-clang-importer-direct-cc1-scan instead? I can crash eliminate_unused_vfs.swift without any changes in this PR when I add -direct-clang-cc1-module-build here. We are crashing in ClangImporter::getClangDriverArguments because of the assert at the beginning of the method.

cachemeifyoucan · 2025-02-13T20:59:59Z

Should I use -experimental-clang-importer-direct-cc1-scan instead? I can crash eliminate_unused_vfs.swift without any changes in this PR when I add -direct-clang-cc1-module-build here.

Yes. My bad. The scanner option is the one you mentioned and it will automatically generate the other flag when compiling interfaces.

…irect-cc1-scan and renaming a function.

cachemeifyoucan

Thanks. LGTM

cachemeifyoucan · 2025-02-13T22:05:12Z

@swift-ci please smoke test

qiongsiwu · 2025-02-14T01:18:47Z

@swift-ci please smoke test

…tput Path (#79752) #79297 implemented current working directory pruning but left some unnecessary code that computes Swift interface module output path prematurely. This PR removes the code that computes the output path too early. The `ExplicitModuleDependencyResolver` now adds the path to the command line after it can correctly compute it. Context: https://github.com/swiftlang/swift/pull/79297/files#r1955314542

qiongsiwu requested review from artemcm, tshortli, cachemeifyoucan, hborla, slavapestov and xedin as code owners February 11, 2025 18:52

qiongsiwu commented Feb 11, 2025

View reviewed changes

include/swift/AST/ModuleDependencies.h Outdated Show resolved Hide resolved

lib/Frontend/ModuleInterfaceLoader.cpp Outdated Show resolved Hide resolved

qiongsiwu requested a review from xymus as a code owner February 12, 2025 20:03

qiongsiwu commented Feb 12, 2025

View reviewed changes

lib/Serialization/ScanningLoaders.cpp Show resolved Hide resolved

qiongsiwu added 7 commits February 12, 2025 20:19

Initial commit of InterfaceModuleNameExpander implmentation. All test…

8055c1d

…s pass, and the unused vfs overlay is optimized.

Refactoring swift interface module's output path calculation to use n…

cd6066d

…ew code.

Adding a test.

15726fc

Address code review comments.

1ce6949

Modify ModuleDependencyInfo so we can set swift interface module outp…

9a5a83a

…ut paths consistently and avoid const_casts.

Removing unused functions.

69e88b1

Addressing code review. Removed the helper class, hence we do not nee…

b7b2795

…d to manage any states.

qiongsiwu force-pushed the upstream_rehash_after_vfs_overlay_prune branch from 22e2531 to b7b2795 Compare February 13, 2025 04:19

artemcm approved these changes Feb 13, 2025

View reviewed changes

cachemeifyoucan reviewed Feb 13, 2025

View reviewed changes

Address code review: adding a test for -experimental-clang-importer-d…

23e863d

…irect-cc1-scan and renaming a function.

cachemeifyoucan approved these changes Feb 13, 2025

View reviewed changes

artemcm merged commit e0359f7 into swiftlang:main Feb 14, 2025
3 checks passed

qiongsiwu mentioned this pull request Mar 3, 2025

[Dependency Scanning] Remove Unnecessary Code that Computes Module Output Path #79752

Merged


		using ArgListTy = std::vector<std::string>;

		void getOutputPath(ResultTy &outputPath, const StringRef &moduleName,

[Dependency Scanning] Update Swift Interface Module's Output Path after vfs Pruning #79297

[Dependency Scanning] Update Swift Interface Module's Output Path after vfs Pruning #79297

Uh oh!

Conversation

qiongsiwu commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qiongsiwu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cachemeifyoucan commented Feb 12, 2025

Uh oh!

qiongsiwu commented Feb 12, 2025

Uh oh!

cachemeifyoucan commented Feb 12, 2025

Uh oh!

Uh oh!

qiongsiwu commented Feb 12, 2025

Uh oh!

qiongsiwu commented Feb 13, 2025

Uh oh!

qiongsiwu commented Feb 13, 2025

Uh oh!

artemcm commented Feb 13, 2025

Uh oh!

cachemeifyoucan left a comment

Choose a reason for hiding this comment

Uh oh!

cachemeifyoucan Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

cachemeifyoucan Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

cachemeifyoucan Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

qiongsiwu Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cachemeifyoucan Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

qiongsiwu Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

qiongsiwu Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

qiongsiwu commented Feb 13, 2025

Uh oh!

cachemeifyoucan commented Feb 13, 2025

Uh oh!

cachemeifyoucan left a comment

Choose a reason for hiding this comment

Uh oh!

cachemeifyoucan commented Feb 13, 2025

Uh oh!

qiongsiwu commented Feb 14, 2025

Uh oh!

Uh oh!

Uh oh!

[Dependency Scanning] Update Swift Interface Module's Output Path after `vfs` Pruning #79297

[Dependency Scanning] Update Swift Interface Module's Output Path after `vfs` Pruning #79297

qiongsiwu commented Feb 11, 2025 •

edited

Loading

qiongsiwu Feb 13, 2025 •

edited

Loading