[Index] Force indexing of system modules to read only from swiftinterfaces #61649

xymus · 2022-10-20T21:05:56Z

Indexing system modules while building is a crash prone phase because it tries to read everything from swiftmodules in the SDK. We saw it crash often on reading implementation details relying on implementation-only imported types.

To rule out all of these issues, let's force indexing system modules to use only swiftinterfaces in the SDK and ignores the adjacent swiftmodule files. In the future, we want a similar behavior for all compilation, not just indexing, at that time we can revert this change.

rdar://100644036

xymus · 2022-10-20T21:06:07Z

@swift-ci Please smoke test

benlangmuir · 2022-10-21T17:12:54Z

Am I correct in understanding that we will still load prebuilt binary modules from the sdk (in the separate prebuilt directory), just not adjacent ones?

benlangmuir · 2022-10-21T17:44:39Z

lib/Index/IndexRecord.cpp

+    module->getASTContext().setIgnoreAdjacentModules(true);
+
+    ImportPath::Module::Builder builder(module->getName());
+    auto reloadedModule = module->getASTContext().getModule(builder.get());


I'm concerned about the cost of re-reading the module here. Even if nothing is later deserialized from the module, my understanding is that loading the module has a non-trivial cost (this was important in code-completion for example). Indexing-while-building is enabled by default in all debug builds, so this would have a wide impact.

Can we move the reload of the module below the parentUnitWriter.isUnitUpToDateForOutputFile check below? Then we would only incur this cost when we will actually need to index the given module. We would still need the updated module filename for that check, but not to actually read the file.

Good point, I moved the new logic after the "Up to date check". I also tweaked it in the hope the we still write down something when the swiftinterface fails to build, so the next indexing phase won't attempt to rebuild it again.

I'm not sure how to test superfluous reindexing though, please let me know if you have an idea.

I'm not sure how to test superfluous reindexing though, please let me know if you have an idea.

We could add a remark (indexing-system-module), add -Rindexing-system-module to the test, and then check the output of the second compile doesn't contain it?

Good idea, I added this remark and used it in the test. It shows on attempts to index, whenever we write a file, and specifies if the content is fixed because of a broken swiftinterface.

xymus · 2022-10-21T19:29:18Z

Pushing a new version to run tests. I still have some offline comments to look into.

@swift-ci Please smoke test

xymus · 2022-10-21T20:12:43Z

Am I correct in understanding that we will still load prebuilt binary modules from the sdk (in the separate prebuilt directory), just not adjacent ones?

That's correct, only the adjacent modules are ignored.

xymus · 2022-10-21T20:14:59Z

@swift-ci Please smoke test

lib/Index/IndexRecord.cpp

bnbarham · 2022-10-21T20:41:18Z

lib/Index/IndexRecord.cpp

+    module->getASTContext().setIgnoreAdjacentModules(true);
+
+    ImportPath::Module::Builder builder(module->getName());
+    auto reloadedModule = module->getASTContext().getModule(builder.get());


I'm not sure how to test superfluous reindexing though, please let me know if you have an idea.

We could add a remark (indexing-system-module), add -Rindexing-system-module to the test, and then check the output of the second compile doesn't contain it?

lib/Index/IndexRecord.cpp

lib/Serialization/SerializedModuleLoader.cpp

xymus · 2022-10-21T23:11:27Z

@swift-ci Please smoke test

bnbarham

Thanks Alexis! This seems reasonable to me, especially if ignoring the adjacent swiftmodule is something we're moving towards for the build anyway.

With the change to not attempt to reload broken modules, the only slowdown now will be the initial reload-from-interface for each module 👍. I think that's a decent tradeoff to avoid all the crashers we've been seeing.

lib/Index/IndexRecord.cpp

lib/Serialization/SerializedModuleLoader.cpp

bnbarham · 2022-10-21T23:23:51Z

test/Index/index_system_modules_swiftinterfaces.swift

+// RUN:     -Rindexing-system-module \
+// RUN:     %t/Client.swift \
+// RUN:     2>&1 | %FileCheck -check-prefix=BROKEN-BUILD-2 --allow-empty %s
+// BROKEN-BUILD-2-NOT: indexing system module


It's working nicely!

xymus · 2022-10-21T23:33:47Z

@swift-ci Please smoke test

xymus · 2022-10-24T00:04:26Z

@swift-ci Please smoke test

xymus · 2022-10-24T01:19:46Z

@swift-ci Please smoke test macOS

xymus · 2022-10-24T16:41:34Z

@swift-ci Please smoke test macOS

xymus · 2022-10-25T17:38:26Z

Tweaked how this interacts with the in-memory cached modules. Instead of dropping the module cache it nows ignores the cache only to reload the target system module to index, and it doesn't attempt to reload the stdlib as it can break existing references to it.

@swift-ci Please smoke test

bnbarham · 2022-10-25T17:50:46Z

include/swift/AST/ASTContext.h

+  /// Change the behavior of all loaders to ignore swiftmodules next to
+  /// swiftinterfaces and clear the in-memory cache on a change of value.


This comment is out of date now, ie. it doesn't clear the in-memory cache.

bnbarham · 2022-10-25T18:08:25Z

lib/AST/ASTContext.cpp

@@ -2441,11 +2445,12 @@ bool ASTContext::canImportModule(ImportPath::Module ModuleName,
 }

 ModuleDecl *
-ASTContext::getModule(ImportPath::Module ModulePath) {
+ASTContext::getModule(ImportPath::Module ModulePath, bool AllowMemoryCached) {


Should we not add the module to the loaded modules when AllowMemoryCached is set? Ie. SerializedModuleLoader.cpp will call Ctx.addLoadedModule(M); after loading the module and that will replace the already loaded one. But that means that if index module A and then module B, which depends on A, we would then have the loaded-from-interface module A when indexing B.

Or does not adding the module cause issues later, eg. something could be expecting to find that module in the loaded modules map and then getting the old B when we're indexing the loaded-from-interface B?

Mostly I'm concerned that we're not expecting this type of thing to happen in the compiler, so it's possible something is caching results that will cause problems here. I don't know if that's true. Though the comment about reloading the stdlib breaking references would point to something like this.

I agree with you about the potential problem here. If the current approach works, I'll look into adding what you suggest, at least for completeness purposes and it may be more important when indexing larger projects.

xymus · 2022-10-25T19:19:41Z

@swift-ci Please smoke test

xymus · 2022-10-26T01:03:32Z

@swift-ci Please smoke test macOS

xymus · 2022-10-26T23:40:28Z

@swift-ci Please smoke test

xymus · 2022-10-27T16:10:23Z

@swift-ci Please smoke test

xymus · 2022-10-27T21:01:16Z

swiftlang/swift-package-manager#5851

@swift-ci Please smoke test macOS

xymus · 2022-10-28T16:35:10Z

Extracted some of the new serialization logic to a different PR and rebased on it.

swiftlang/swift-package-manager#5851

@swift-ci Please smoke test

xymus · 2022-10-28T20:23:27Z

@swift-ci Please smoke test Linux

include/swift/AST/Decl.h

include/swift/AST/Module.h

include/swift/ClangImporter/ClangImporter.h

lib/Frontend/ModuleInterfaceBuilder.cpp

lib/Frontend/ModuleInterfaceLoader.cpp

lib/FrontendTool/FrontendTool.cpp

lib/Serialization/ModuleFileSharedCore.h

xymus · 2022-10-30T15:42:32Z

Cannot contact i-09a88659ae06fbf33: hudson.remoting.ChannelClosedException: Channel "hudson.remoting.Channel@3cbfe351:i-09a88659ae06fbf33": Remote call on i-09a88659ae06fbf33 failed. The channel is closing down or has closed down

@swift-ci Please smoke test Linux

Intro ASTContext::setIgnoreAdjacentModules to change module loading to accept load only resilient modules from their swiftinterfaces, ignoring the adjacent module and any silencing swiftinterfaces errors.

Use setIgnoreAdjacentModules before indexing any resilient system module to force indexing to read only from resilient modules. rdar://100644036

Use a clean module cache in tests printing path to the swiftmodule/swiftinterface used as it may change depending on previous tests.

…al.swift

…ory cache

…gine

xymus · 2022-10-31T18:12:53Z

@swift-ci Please smoke test

xymus requested a review from bnbarham October 20, 2022 21:05

benlangmuir reviewed Oct 21, 2022

View reviewed changes

xymus force-pushed the index-swiftinterfaces branch from 2c1a719 to 7b8547b Compare October 21, 2022 19:28

bnbarham reviewed Oct 21, 2022

View reviewed changes

xymus force-pushed the index-swiftinterfaces branch from 5bef4c3 to fa80f22 Compare October 21, 2022 23:11

bnbarham approved these changes Oct 21, 2022

View reviewed changes

xymus force-pushed the index-swiftinterfaces branch from fa80f22 to 0ed635c Compare October 21, 2022 23:33

xymus force-pushed the index-swiftinterfaces branch from d89865f to 1538fc3 Compare October 25, 2022 17:35

bnbarham reviewed Oct 25, 2022

View reviewed changes

xymus force-pushed the index-swiftinterfaces branch from 1538fc3 to 34ce961 Compare October 26, 2022 23:40

xymus force-pushed the index-swiftinterfaces branch from 34ce961 to ab3c186 Compare October 27, 2022 16:10

xymus mentioned this pull request Oct 28, 2022

[Serialization] Write in the binary swiftmodule file whether it was rebuilt from a swiftinterface #61789

Merged

xymus force-pushed the index-swiftinterfaces branch from ab3c186 to 009149a Compare October 28, 2022 16:34

benlangmuir reviewed Oct 28, 2022

View reviewed changes

xymus force-pushed the index-swiftinterfaces branch from 009149a to 1d46ca0 Compare October 31, 2022 17:43

xymus added 14 commits October 31, 2022 10:58

[Frontend] Intro global control to force loading from swiftinterface

83cd432

Intro ASTContext::setIgnoreAdjacentModules to change module loading to accept load only resilient modules from their swiftinterfaces, ignoring the adjacent module and any silencing swiftinterfaces errors.

[Index] Force indexing from the swiftinterfaces of system modules

01d35f3

Use setIgnoreAdjacentModules before indexing any resilient system module to force indexing to read only from resilient modules. rdar://100644036

[Test] Use a clean module cache for more tests

fec0498

Use a clean module cache in tests printing path to the swiftmodule/swiftinterface used as it may change depending on previous tests.

[Test] Avoid unexpected deserialization failure in skip-loaded-intern…

8e4f62a

…al.swift

[Index] Move reloading from swiftinterface after the "up to date" check

c7de4b1

[Index] Skip indexing modules failing to build but write it down

b250f3e

[Index] Remark on indexing system modules and use it in tests

90feb49

[Index] Apply small comments from PR review

520537e

[Sema|Index] Ignore the cache when loading a system module for indexing

5ed1f12

[Index] Do not reload the stdlib when indexing system modules

3b63f49

[Serialization] Add control over adding a loaded module to the in-mem…

730497e

…ory cache

[Index] Don't reload modules already built from swiftinterface

ae00779

[Serialization] Minor improvements to doc from PR comments

82f63b0

[ModuleInterface] Implement silencing errors on rebuild with a new en…

5d59a8f

…gine

xymus force-pushed the index-swiftinterfaces branch from 1d46ca0 to 5d59a8f Compare October 31, 2022 18:11

benlangmuir approved these changes Oct 31, 2022

View reviewed changes

xymus merged commit 47b29b6 into swiftlang:main Oct 31, 2022

xymus deleted the index-swiftinterfaces branch October 31, 2022 21:23

		/// Change the behavior of all loaders to ignore swiftmodules next to
		/// swiftinterfaces and clear the in-memory cache on a change of value.

[Index] Force indexing of system modules to read only from swiftinterfaces #61649

[Index] Force indexing of system modules to read only from swiftinterfaces #61649

Uh oh!

Conversation

xymus commented Oct 20, 2022

Uh oh!

xymus commented Oct 20, 2022

Uh oh!

benlangmuir commented Oct 21, 2022

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!

xymus commented Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xymus commented Oct 21, 2022

Uh oh!

xymus commented Oct 21, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xymus commented Oct 21, 2022

Uh oh!

bnbarham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xymus commented Oct 21, 2022

Uh oh!

xymus commented Oct 24, 2022

Uh oh!

xymus commented Oct 24, 2022

Uh oh!

xymus commented Oct 24, 2022

Uh oh!

xymus commented Oct 25, 2022

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!

xymus commented Oct 25, 2022

Uh oh!

xymus commented Oct 26, 2022

Uh oh!

xymus commented Oct 26, 2022

Uh oh!

xymus commented Oct 27, 2022

Uh oh!

xymus commented Oct 27, 2022

Uh oh!

xymus commented Oct 28, 2022

Uh oh!

xymus commented Oct 28, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xymus commented Oct 21, 2022 •

edited

Loading