[Serialization] Improve module loading performance #40155

ahoppen · 2021-11-12T10:15:04Z

When looking for a Swift module on disk, we were scanning all module search paths if they contain the module we are searching for. In a setup where each module is contained in its own framework search path, this scaled quadratically with the number of modules being imported. E.g. a setup with 100 modules being imported form 100 module search paths could cause on the order of 10,000 checks of FileSystem::exists. While these checks are fairly fast (~10µs), they add up to ~100ms.

To improve this, perform a first scan of all module search paths and list the files they contain. From this, create a lookup map that maps filenames to the search paths they can be found in. E.g. for

searchPath1/
  Module1.framework

searchPath2/
  Module1.framework
  Module2.swiftmodule

we create the following lookup table

Module1.framework -> [searchPath1, searchPath2]
Module2.framework -> [searchPath2]

The initial scan is fairly fast (in O(files inside module search paths)) and afterwards finding a module in the framework search paths is in O(1) in the situation described above.

ahoppen · 2021-11-12T10:15:58Z

@swift-ci Please smoke test

ahoppen · 2021-11-12T15:07:51Z

@swift-ci Please smoke test

lib/Serialization/SerializedModuleLoader.cpp

xymus

This sounds like an overdue improvement, thanks!

bnbarham

Nice! Those microseconds really add up 😆

include/swift/Serialization/SerializedModuleLoader.h

lib/Serialization/ModuleDependencyScanner.cpp

lib/Serialization/ModuleFile.cpp

lib/Serialization/SerializedModuleLoader.cpp

ahoppen · 2021-11-16T18:13:06Z

OK, I needed to do a pretty big refactoring of the PR because before each module loader maintained its own lookup table. Thus, if one module loader caused a search path to get added, the others wouldn’t know about it. So I moved the entire lookup logic down to SearchPathOpts, which has a better overview of any search path modifications anyway. For that I needed to make sure that all modifications to the module search paths are made through dedicated setters that can update/invalidate the lookup.

bnbarham · 2021-11-16T23:36:23Z

lib/AST/SearchPathOptions.cpp

+       !Error && Dir != llvm::vfs::directory_iterator(); Dir.increment(Error)) {
+    StringRef Filename = llvm::sys::path::filename(Dir->path());
+    LookupTable[Filename].push_back(
+        {SearchPath.str(), Kind, IsSystem, SearchPathIndex});


If we store the results of getDarwinImplicitFrameworkSearchPaths in SearchPathOptions this could then be a StringRef (I think). Possibly worth it?

With the current design, we only decide whether we need these paths (i.e. whether we are on a Darwin platform) when enumerating the search paths. So if we want to store the Darwin implicit framework search paths in SearchPathOptions, we would need to always compute them when setting SDK path, regardless of the platform (or have a slightly more complicated caching method). I’m not sure if it’s worth it. My current gut feeling is that this function will be called <10 times per compilation/SourceKit operation.

The clean solution would be that SearchPathOptions knows whether it’s on a Darwin platform, but I think that’s out of scope for this PR.

Just seemed like a relatively large map to me, but I guess there's not that many file -> search paths. I was mostly hoping we could move the platform check into ParseSearchPathArgs and just add the extra paths directly to the framework search paths rather than have separate field, but I haven't looked into that at all. If that's not possible then I don't really have a preference either way 🤷

Ah, sorry. I misunderstood you. I though you wanted to avoid the computation of getDarwinImplicitFrameworkSearchPaths. Making ModuleSearchPath only contain a StringRef does seem like a worthwhile improvement 👍

I was mostly hoping we could move the platform check into ParseSearchPathArgs and just add the extra paths directly to the framework search paths rather than have separate field, but I haven't looked into that at all. If that's not possible then I don't really have a preference either way 🤷

We can’t do that because framework search paths may be added after the compiler invocation is created and those should come before the Darwin implicit search paths.

Ah I see. Yeah the computation didn't really bother me at all - from a quick look this just seemed to be the only part that was preventing using StringRefs.

bnbarham · 2021-11-16T23:39:50Z

lib/ClangImporter/ClangImporter.cpp

-      for (auto path : searchPathOpts.ImportSearchPaths) {
+      for (auto path : searchPathOpts.getImportSearchPaths()) {


I know you only changed to the getter here, but it's always nice to be explicit with const auto &. There's a few others where the same applies.

bnbarham · 2021-11-16T23:41:09Z

lib/Serialization/SerializedModuleLoader.cpp

+      isFramework = false;
+
+      // On Apple platforms, we can assume that the runtime libraries use
+      // target-specifi module files wihtin a `.swiftmodule` directory.


I know you didn't actually change any of this but:
target-specifi -> target-specific
wihtin -> within

bnbarham · 2021-11-16T23:44:36Z

lib/AST/SearchPathOptions.cpp

+  // Gather all search paths that include a file whose name is in Filenames.
+  // To make sure that we don't include search paths twice, keep track of which
+  // search paths have already been added to Result by their kind and Index
+  // in ResultIds.
+  llvm::SmallVector<const ModuleSearchPath *, 4> Result;
+  llvm::SmallSet<std::pair<ModuleSearchPathKind, unsigned>, 4> ResultIds;


We can still add a search path twice now right?

Yes, if it’s specified twice on the command line (and not cleaned up earlier). Is that good or bad or are you just wondering? Search paths could also exist twice as different kinds, e.g. once as framework search path and once as import search path.

I was mostly just pointing out To make sure that we don't include search paths twice but it is clarified later in that sentence so 🤷

Oh, yes. Good catch.

ahoppen · 2021-11-22T20:09:33Z

swiftlang/llvm-project#3546

@swift-ci Please smoke test

ahoppen · 2021-12-01T21:05:11Z

@swift-ci Please test source compatibility

ahoppen · 2021-12-08T21:55:16Z

@swift-ci Please smoke test

ahoppen · 2021-12-10T09:00:08Z

swiftlang/llvm-project#3546

@swift-ci Please smoke test

ahoppen · 2021-12-13T14:30:49Z

swiftlang/llvm-project#3546

@swift-ci Please smoke test

When looking for a Swift module on disk, we were scanning all module search paths if they contain the module we are searching for. In a setup where each module is contained in its own framework search path, this scaled quadratically with the number of modules being imported. E.g. a setup with 100 modules being imported form 100 module search paths could cause on the order of 10,000 checks of `FileSystem::exists`. While these checks are fairly fast (~10µs), they add up to ~100ms. To improve this, perform a first scan of all module search paths and list the files they contain. From this, create a lookup map that maps filenames to the search paths they can be found in. E.g. for ``` searchPath1/ Module1.framework searchPath2/ Module1.framework Module2.swiftmodule ``` we create the following lookup table ``` Module1.framework -> [searchPath1, searchPath2] Module2.swiftmodule -> [searchPath2] ```

ahoppen · 2021-12-14T11:45:02Z

swiftlang/llvm-project#3546

@swift-ci Please smoke test

nkcsgexi · 2021-12-21T20:09:31Z

@ahoppen sorry that I have to revert it despite this is a very nice change: #40663

ahoppen requested a review from bnbarham November 12, 2021 10:15

ahoppen requested a review from xymus November 12, 2021 10:17

ahoppen force-pushed the pr/improve-module-search-path-lookup branch from f9327ac to 6e95156 Compare November 12, 2021 15:07

xymus reviewed Nov 12, 2021

View reviewed changes

lib/Serialization/SerializedModuleLoader.cpp Show resolved Hide resolved

xymus approved these changes Nov 12, 2021

View reviewed changes

bnbarham reviewed Nov 13, 2021

View reviewed changes

ahoppen force-pushed the pr/improve-module-search-path-lookup branch from 6e95156 to 3a0106e Compare November 16, 2021 18:07

ahoppen force-pushed the pr/improve-module-search-path-lookup branch 2 times, most recently from 738e9b7 to 4acc427 Compare November 16, 2021 20:52

ahoppen mentioned this pull request Nov 16, 2021

[lldb] Use getters to retrieve Swift module search paths swiftlang/llvm-project#3546

Merged

bnbarham reviewed Nov 16, 2021

View reviewed changes

ahoppen force-pushed the pr/improve-module-search-path-lookup branch 5 times, most recently from 513885a to 67efb25 Compare November 22, 2021 20:09

ahoppen force-pushed the pr/improve-module-search-path-lookup branch from 67efb25 to e6634da Compare December 8, 2021 21:55

ahoppen force-pushed the pr/improve-module-search-path-lookup branch 2 times, most recently from 89180c6 to bb55152 Compare December 13, 2021 14:30

ahoppen force-pushed the pr/improve-module-search-path-lookup branch from bb55152 to fe7878e Compare December 14, 2021 11:44

ahoppen merged commit 669e3f3 into swiftlang:main Dec 20, 2021

ahoppen deleted the pr/improve-module-search-path-lookup branch December 20, 2021 17:09

nkcsgexi mentioned this pull request Dec 21, 2021

Revert "[Serialization] Improve module loading performance" #40663

Closed

ahoppen mentioned this pull request Feb 19, 2022

[Serialization] Store the path inside ModuleSearchPath as a string, not a StringRef #41476

Merged

		for (auto path : searchPathOpts.ImportSearchPaths) {
		for (auto path : searchPathOpts.getImportSearchPaths()) {

[Serialization] Improve module loading performance #40155

[Serialization] Improve module loading performance #40155

Uh oh!

Conversation

ahoppen commented Nov 12, 2021

Uh oh!

ahoppen commented Nov 12, 2021

Uh oh!

ahoppen commented Nov 12, 2021

Uh oh!

Uh oh!

xymus left a comment

Choose a reason for hiding this comment

Uh oh!

bnbarham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahoppen commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Nov 22, 2021

Uh oh!

ahoppen commented Dec 1, 2021

Uh oh!

ahoppen commented Dec 8, 2021

Uh oh!

ahoppen commented Dec 10, 2021

Uh oh!

ahoppen commented Dec 13, 2021

Uh oh!

ahoppen commented Dec 14, 2021

Uh oh!

nkcsgexi commented Dec 21, 2021

Uh oh!

Uh oh!

ahoppen commented Nov 16, 2021 •

edited

Loading