[SymbolGraph] don't scan every available module for cross-import overlays #36303
Conversation
|
@swift-ci Please smoke test |
|
@swift-ci Please Build Toolchain macOS Platform |
|
|
||
| Success |= symbolgraphgen::emitSymbolGraphForModule(CIM, Options); | ||
| } | ||
| SmallVector<ModuleDecl *, 2> Overlays; |
There was a problem hiding this comment.
Any specific reason for 2? Is it meant to be 32?
There was a problem hiding this comment.
No specific reason, other than "i thought the template parameter was required". It looks like i can leave the template parameter out entirely and get a "reasonable default", so i'll do that.
There was a problem hiding this comment.
TIL and I wish I’d known this years ago
| @@ -260,16 +260,16 @@ int swift_symbolgraph_extract_main(ArrayRef<const char *> Args, const char *Argv | |||
| // don't need to print these errors. | |||
| CI.removeDiagnosticConsumer(&DiagPrinter); | |||
|
|
|||
| for (const auto &ModuleName : VisibleModuleNames) { | |||
There was a problem hiding this comment.
Do we still need to accumulate the VisibleModuleNames above? Maybe we should log what findDeclaredCrossImportOverlaysTransitive returns?
There was a problem hiding this comment.
As for logging, since we're printing the modules as we load them, i don't think it's 100% necessary to add more logging on top of that just to list the modules beforehand.
| } | ||
| SmallVector<ModuleDecl *, 2> Overlays; | ||
| M->findDeclaredCrossImportOverlaysTransitive(Overlays); | ||
| for (const auto *OM : Overlays) { |
There was a problem hiding this comment.
Looks like we used to sort the overlays by name. Is this something we should do here as well? I'm not sure whether sorting was a requirement for us to produce correct SGFs.
There was a problem hiding this comment.
The original use for VisibleModuleNames is for reporting to the user when the given module name wasn't found. The sorting was (as far as i know) just for ease of user experience when reading the module list. I can move the initialization and sorting back into that error branch so we don't hit that in the normal path at all.
|
I've pushed a couple commits for the review comments. @swift-ci Please smoke test |
|
@swift-ci Please Build Toolchain macOS Platform |
QuietMisdreavus
force-pushed
the
QuietMisdreavus/better-overlay-search
branch
from
69109bc to
9717af8
Compare
|
@swift-ci Please smoke test |
|
macOS Toolchain Install command |
|
Linux CI seems to have failed on an @swift-ci Please smoke test Linux platform |
|
Looks like i got hit by some test failures that were also happening on Friday morning. From what i can tell, the test in question has been reverted by now, so once more, with feeling... @swift-ci Please smoke test Linux platform |
Resolves rdar://74928853
When the symbol graph tool checks for cross-import overlays declared by the current module, it currently looks through all the available modules, and loads every module whose name begins with an underscore to check it. This can lead to a wildly inefficient situation if there are many large overlay modules in the SDK. In one situation the symbol graph tool tool two minutes on my machine for a module with only a handful of items in it, just because of the overlay module scan.
This PR changes this behavior to instead query the module itself for any overlay modules, the same way that SourceKit would. This speeds up that module's call to a fraction of a second, providing a three order-of-magnitude speedup to the call overall.