Skip to content

[SourceKit] Use a single PluginRegistry in multiple ASTContexts #64655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/swift/AST/ASTContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -1503,6 +1503,8 @@ class ASTContext final {
/// This should be called before any plugin is loaded.
void setPluginRegistry(PluginRegistry *newValue);

const llvm::StringSet<> &getLoadedPluginLibraryPaths() const;

private:
friend Decl;

Expand Down
6 changes: 2 additions & 4 deletions include/swift/AST/PluginRegistry.h
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,8 @@ class PluginRegistry {
/// Flag to dump plugin messagings.
bool dumpMessaging = false;

std::mutex mtx;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getLoadedLibraryPlugins() needs to lock this, too.

Copy link
Member Author

@rintaro rintaro Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to remove getLoadedLibraryPlugins() method in follow-ups soon.
Currently only swift::emitLoadedModuleTraceIfNeeded() is using it for dependency tracking. However since PluginRegistry might be shared between different compilations, getLoadedLibraryPlugins() may contain unrelated entries. Instead, each ASTContext should track loaded libraries separately.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a commit in this PR removing PluginRegistry::getLoadedLibraryPlugins().


public:
PluginRegistry();

Expand All @@ -150,10 +152,6 @@ class PluginRegistry {
/// If \p path plugin is already loaded, this returns the cached object.
llvm::Expected<LoadedExecutablePlugin *>
loadExecutablePlugin(llvm::StringRef path);

const llvm::StringMap<void *> &getLoadedLibraryPlugins() const {
return LoadedPluginLibraries;
}
};

} // namespace swift
7 changes: 5 additions & 2 deletions include/swift/IDETool/CompileInstance.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,15 @@ namespace swift {

class CompilerInstance;
class DiagnosticConsumer;
class PluginRegistry;

namespace ide {

/// Manages \c CompilerInstance for completion like operations.
class CompileInstance {
const std::string &RuntimeResourcePath;
const std::string &DiagnosticDocumentationPath;
const std::shared_ptr<swift::PluginRegistry> Plugins;

struct Options {
unsigned MaxASTReuseCount = 100;
Expand Down Expand Up @@ -66,10 +68,11 @@ class CompileInstance {

public:
CompileInstance(const std::string &RuntimeResourcePath,
const std::string &DiagnosticDocumentationPath)
const std::string &DiagnosticDocumentationPath,
std::shared_ptr<swift::PluginRegistry> Plugins = nullptr)
: RuntimeResourcePath(RuntimeResourcePath),
DiagnosticDocumentationPath(DiagnosticDocumentationPath),
CachedCIInvalidated(false), CachedReuseCount(0) {}
Plugins(Plugins), CachedCIInvalidated(false), CachedReuseCount(0) {}

/// NOTE: \p Args is only used for checking the equaity of the invocation.
/// Since this function assumes that it is already normalized, exact the same
Expand Down
6 changes: 5 additions & 1 deletion include/swift/IDETool/IDEInspectionInstance.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ namespace swift {
class CompilerInstance;
class CompilerInvocation;
class DiagnosticConsumer;
class PluginRegistry;

namespace ide {

Expand Down Expand Up @@ -96,6 +97,8 @@ class IDEInspectionInstance {

std::mutex mtx;

std::shared_ptr<PluginRegistry> Plugins;

std::shared_ptr<CompilerInstance> CachedCI;
llvm::hash_code CachedArgHash;
llvm::sys::TimePoint<> DependencyCheckedTimestamp;
Expand Down Expand Up @@ -167,7 +170,8 @@ class IDEInspectionInstance {
Callback);

public:
IDEInspectionInstance() : CachedCIShouldBeInvalidated(false) {}
IDEInspectionInstance(std::shared_ptr<PluginRegistry> Plugins = nullptr)
: Plugins(Plugins), CachedCIShouldBeInvalidated(false) {}

// Mark the cached compiler instance "should be invalidated". In the next
// completion, new compiler instance will be used. (Thread safe.)
Expand Down
9 changes: 9 additions & 0 deletions lib/AST/ASTContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,8 @@ struct ASTContext::Implementation {
/// Map a module name to an executable plugin path that provides the module.
llvm::DenseMap<Identifier, StringRef> ExecutablePluginPaths;

llvm::StringSet<> LoadedPluginLibraryPaths;

/// The permanent arena.
Arena Permanent;

Expand Down Expand Up @@ -6395,6 +6397,9 @@ LoadedExecutablePlugin *ASTContext::loadExecutablePlugin(StringRef path) {
}

void *ASTContext::loadLibraryPlugin(StringRef path) {
// Remember the path (even if it fails to load.)
getImpl().LoadedPluginLibraryPaths.insert(path);
Comment on lines 6399 to +6401
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to track failed libraries? If not, I can just move this after getPluginRegistry()->loadLibraryPlugin(resolvedPath) .

Also, do we want the VFS path or the "real" path here? I guess it doesn't matter because we only use this to get the module name from the filename. cc: @rxwei


SmallString<128> resolvedPath;
auto fs = this->SourceMgr.getFileSystem();
if (auto err = fs->getRealPath(path, resolvedPath)) {
Expand All @@ -6413,6 +6418,10 @@ void *ASTContext::loadLibraryPlugin(StringRef path) {
return plugin.get();
}

const llvm::StringSet<> &ASTContext::getLoadedPluginLibraryPaths() const {
return getImpl().LoadedPluginLibraryPaths;
}

bool ASTContext::supportsMoveOnlyTypes() const {
// currently the only thing holding back whether the types can appear is this.
return SILOpts.LexicalLifetimes != LexicalLifetimesOption::Off;
Expand Down
3 changes: 3 additions & 0 deletions lib/AST/PluginRegistry.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ PluginRegistry::PluginRegistry() {
}

llvm::Expected<void *> PluginRegistry::loadLibraryPlugin(StringRef path) {
std::lock_guard<std::mutex> lock(mtx);
auto found = LoadedPluginLibraries.find(path);
if (found != LoadedPluginLibraries.end()) {
// Already loaded.
Expand Down Expand Up @@ -74,6 +75,8 @@ PluginRegistry::loadExecutablePlugin(StringRef path) {
return llvm::errorCodeToError(err);
}

std::lock_guard<std::mutex> lock(mtx);

// See if the plugin is already loaded.
auto &storage = LoadedPluginExecutables[path];
if (storage) {
Expand Down
3 changes: 1 addition & 2 deletions lib/FrontendTool/LoadedModuleTrace.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -760,8 +760,7 @@ bool swift::emitLoadedModuleTraceIfNeeded(ModuleDecl *mainModule,
}

// Add compiler plugin libraries as dependencies.
auto *pluginRegistry = ctxt.getPluginRegistry();
for (auto &pluginEntry : pluginRegistry->getLoadedLibraryPlugins())
for (auto &pluginEntry : ctxt.getLoadedPluginLibraryPaths())
depTracker->addDependency(pluginEntry.getKey(), /*IsSystem*/ false);

std::vector<SwiftModuleTraceInfo> swiftModules;
Expand Down
1 change: 1 addition & 0 deletions lib/IDETool/CompileInstance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ bool CompileInstance::setupCI(
assert(Diags.hadAnyError());
return false;
}
CI->getASTContext().setPluginRegistry(Plugins.get());

return true;
}
Expand Down
1 change: 1 addition & 0 deletions lib/IDETool/IDEInspectionInstance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,7 @@ void IDEInspectionInstance::performNewOperation(
InstanceSetupError));
return;
}
CI->getASTContext().setPluginRegistry(Plugins.get());
CI->getASTContext().CancellationFlag = CancellationFlag;
registerIDERequestFunctions(CI->getASTContext().evaluator);

Expand Down
13 changes: 9 additions & 4 deletions tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -553,10 +553,12 @@ struct SwiftASTManager::Implementation {
std::shared_ptr<SwiftEditorDocumentFileMap> EditorDocs,
std::shared_ptr<GlobalConfig> Config,
std::shared_ptr<SwiftStatistics> Stats,
std::shared_ptr<RequestTracker> ReqTracker, StringRef SwiftExecutablePath,
std::shared_ptr<RequestTracker> ReqTracker,
std::shared_ptr<PluginRegistry> Plugins, StringRef SwiftExecutablePath,
StringRef RuntimeResourcePath, StringRef DiagnosticDocumentationPath)
: EditorDocs(EditorDocs), Config(Config), Stats(Stats),
ReqTracker(ReqTracker), SwiftExecutablePath(SwiftExecutablePath),
ReqTracker(ReqTracker), Plugins(Plugins),
SwiftExecutablePath(SwiftExecutablePath),
RuntimeResourcePath(RuntimeResourcePath),
DiagnosticDocumentationPath(DiagnosticDocumentationPath),
SessionTimestamp(llvm::sys::toTimeT(std::chrono::system_clock::now())) {
Expand All @@ -566,6 +568,7 @@ struct SwiftASTManager::Implementation {
std::shared_ptr<GlobalConfig> Config;
std::shared_ptr<SwiftStatistics> Stats;
std::shared_ptr<RequestTracker> ReqTracker;
std::shared_ptr<PluginRegistry> Plugins;
/// The path of the swift-frontend executable.
/// Used to find clang relative to it.
std::string SwiftExecutablePath;
Expand Down Expand Up @@ -638,9 +641,10 @@ SwiftASTManager::SwiftASTManager(
std::shared_ptr<SwiftEditorDocumentFileMap> EditorDocs,
std::shared_ptr<GlobalConfig> Config,
std::shared_ptr<SwiftStatistics> Stats,
std::shared_ptr<RequestTracker> ReqTracker, StringRef SwiftExecutablePath,
std::shared_ptr<RequestTracker> ReqTracker,
std::shared_ptr<PluginRegistry> Plugins, StringRef SwiftExecutablePath,
StringRef RuntimeResourcePath, StringRef DiagnosticDocumentationPath)
: Impl(*new Implementation(EditorDocs, Config, Stats, ReqTracker,
: Impl(*new Implementation(EditorDocs, Config, Stats, ReqTracker, Plugins,
SwiftExecutablePath, RuntimeResourcePath,
DiagnosticDocumentationPath)) {}

Expand Down Expand Up @@ -1073,6 +1077,7 @@ ASTUnitRef ASTBuildOperation::buildASTUnit(std::string &Error) {
}
return nullptr;
}
CompIns.getASTContext().setPluginRegistry(ASTManager->Impl.Plugins.get());
CompIns.getASTContext().CancellationFlag = CancellationFlag;
registerIDERequestFunctions(CompIns.getASTContext().evaluator);
if (TracedOp.enabled()) {
Expand Down
2 changes: 2 additions & 0 deletions tools/SourceKit/lib/SwiftLang/SwiftASTManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ namespace swift {
class CompilerInstance;
class CompilerInvocation;
class DiagnosticEngine;
class PluginRegistry;
class SourceFile;
class SourceManager;
}
Expand Down Expand Up @@ -235,6 +236,7 @@ class SwiftASTManager : public std::enable_shared_from_this<SwiftASTManager> {
std::shared_ptr<GlobalConfig> Config,
std::shared_ptr<SwiftStatistics> Stats,
std::shared_ptr<RequestTracker> ReqTracker,
std::shared_ptr<swift::PluginRegistry> Plugins,
StringRef SwiftExecutablePath,
StringRef RuntimeResourcePath,
StringRef DiagnosticDocumentationPath);
Expand Down
10 changes: 6 additions & 4 deletions tools/SourceKit/lib/SwiftLang/SwiftCompile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ compile::SessionManager::getSession(StringRef name) {
}

bool inserted = false;
std::tie(i, inserted) = sessions.try_emplace(name, std::make_shared<compile::Session>(RuntimeResourcePath, DiagnosticDocumentationPath));
std::tie(i, inserted) = sessions.try_emplace(
name, std::make_shared<compile::Session>(
RuntimeResourcePath, DiagnosticDocumentationPath, Plugins));
assert(inserted);
return i->second;
}
Expand Down Expand Up @@ -141,10 +143,10 @@ void SwiftLangSupport::performCompile(
CancellationFlag->store(true, std::memory_order_relaxed);
});

CompileManager.performCompileAsync(Name, Args, std::move(fileSystem),
CancellationFlag, Receiver);
CompileManager->performCompileAsync(Name, Args, std::move(fileSystem),
CancellationFlag, Receiver);
}

void SwiftLangSupport::closeCompile(StringRef Name) {
CompileManager.clearSession(Name);
CompileManager->clearSession(Name);
}
15 changes: 10 additions & 5 deletions tools/SourceKit/lib/SwiftLang/SwiftLangSupport.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -276,24 +276,29 @@ static void configureIDEInspectionInstance(
SwiftLangSupport::SwiftLangSupport(SourceKit::Context &SKCtx)
: NotificationCtr(SKCtx.getNotificationCenter()),
SwiftExecutablePath(SKCtx.getSwiftExecutablePath()),
ReqTracker(SKCtx.getRequestTracker()), CCCache(new SwiftCompletionCache),
CompileManager(RuntimeResourcePath, DiagnosticDocumentationPath) {
ReqTracker(SKCtx.getRequestTracker()), CCCache(new SwiftCompletionCache) {
llvm::SmallString<128> LibPath(SKCtx.getRuntimeLibPath());
llvm::sys::path::append(LibPath, "swift");
RuntimeResourcePath = std::string(LibPath.str());
DiagnosticDocumentationPath = SKCtx.getDiagnosticDocumentationPath().str();

Stats = std::make_shared<SwiftStatistics>();
EditorDocuments = std::make_shared<SwiftEditorDocumentFileMap>();

std::shared_ptr<PluginRegistry> Plugins = std::make_shared<PluginRegistry>();

ASTMgr = std::make_shared<SwiftASTManager>(
EditorDocuments, SKCtx.getGlobalConfiguration(), Stats, ReqTracker,
SwiftExecutablePath, RuntimeResourcePath, DiagnosticDocumentationPath);

IDEInspectionInst = std::make_shared<IDEInspectionInstance>();
Plugins, SwiftExecutablePath, RuntimeResourcePath,
DiagnosticDocumentationPath);

IDEInspectionInst = std::make_shared<IDEInspectionInstance>(Plugins);
configureIDEInspectionInstance(IDEInspectionInst,
SKCtx.getGlobalConfiguration());

CompileManager = std::make_shared<compile::SessionManager>(
RuntimeResourcePath, DiagnosticDocumentationPath, Plugins);

// By default, just use the in-memory cache.
CCCache->inMemory = std::make_unique<ide::CodeCompletionCache>();

Expand Down
14 changes: 9 additions & 5 deletions tools/SourceKit/lib/SwiftLang/SwiftLangSupport.h
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,9 @@ class Session {

public:
Session(const std::string &RuntimeResourcePath,
const std::string &DiagnosticDocumentationPath)
: Compiler(RuntimeResourcePath, DiagnosticDocumentationPath) {}
const std::string &DiagnosticDocumentationPath,
std::shared_ptr<swift::PluginRegistry> Plugins)
: Compiler(RuntimeResourcePath, DiagnosticDocumentationPath, Plugins) {}

bool
performCompile(llvm::ArrayRef<const char *> Args,
Expand All @@ -308,6 +309,7 @@ class Session {
class SessionManager {
const std::string &RuntimeResourcePath;
const std::string &DiagnosticDocumentationPath;
const std::shared_ptr<swift::PluginRegistry> Plugins;

llvm::StringMap<std::shared_ptr<Session>> sessions;
WorkQueue compileQueue{WorkQueue::Dequeuing::Concurrent,
Expand All @@ -316,9 +318,11 @@ class SessionManager {

public:
SessionManager(std::string &RuntimeResourcePath,
std::string &DiagnosticDocumentationPath)
std::string &DiagnosticDocumentationPath,
std::shared_ptr<swift::PluginRegistry> Plugins)
: RuntimeResourcePath(RuntimeResourcePath),
DiagnosticDocumentationPath(DiagnosticDocumentationPath) {}
DiagnosticDocumentationPath(DiagnosticDocumentationPath),
Plugins(Plugins) {}

std::shared_ptr<Session> getSession(StringRef name);

Expand Down Expand Up @@ -356,7 +360,7 @@ class SwiftLangSupport : public LangSupport {
std::shared_ptr<SwiftStatistics> Stats;
llvm::StringMap<std::unique_ptr<FileSystemProvider>> FileSystemProviders;
std::shared_ptr<swift::ide::IDEInspectionInstance> IDEInspectionInst;
compile::SessionManager CompileManager;
std::shared_ptr<compile::SessionManager> CompileManager;

public:
explicit SwiftLangSupport(SourceKit::Context &SKCtx);
Expand Down
11 changes: 7 additions & 4 deletions tools/swift-ide-test/swift-ide-test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "swift/AST/DiagnosticEngine.h"
#include "swift/AST/ImportCache.h"
#include "swift/AST/NameLookupRequests.h"
#include "swift/AST/PluginRegistry.h"
#include "swift/AST/PrintOptions.h"
#include "swift/AST/RawComment.h"
#include "swift/AST/USRGeneration.h"
Expand Down Expand Up @@ -1188,7 +1189,8 @@ static int doTypeContextInfo(const CompilerInvocation &InitInvok,
InitInvok, SourceFilename, SecondSourceFileName, CodeCompletionToken,
CodeCompletionDiagnostics,
[&](CompletionLikeOperationParams Params) -> bool {
IDEInspectionInstance IDEInspectionInst;
IDEInspectionInstance IDEInspectionInst(
std::make_shared<PluginRegistry>());
int ExitCode = 2;
IDEInspectionInst.typeContextInfo(
Params.Invocation, Params.Args, Params.FileSystem,
Expand Down Expand Up @@ -1260,7 +1262,8 @@ doConformingMethodList(const CompilerInvocation &InitInvok,
InitInvok, SourceFilename, SecondSourceFileName, CodeCompletionToken,
CodeCompletionDiagnostics,
[&](CompletionLikeOperationParams Params) -> bool {
IDEInspectionInstance IDEInspectionInst;
IDEInspectionInstance IDEInspectionInst(
std::make_shared<PluginRegistry>());
int ExitCode = 2;
IDEInspectionInst.conformingMethodList(
Params.Invocation, Params.Args, Params.FileSystem,
Expand Down Expand Up @@ -1410,7 +1413,7 @@ doCodeCompletion(const CompilerInvocation &InitInvok, StringRef SourceFilename,
InitInvok, SourceFilename, SecondSourceFileName, CodeCompletionToken,
CodeCompletionDiagnostics,
[&](CompletionLikeOperationParams Params) -> bool {
IDEInspectionInstance Inst;
IDEInspectionInstance Inst(std::make_shared<PluginRegistry>());
int ExitCode = 2;
Inst.codeComplete(
Params.Invocation, Params.Args, Params.FileSystem,
Expand Down Expand Up @@ -1504,7 +1507,7 @@ static int doBatchCodeCompletion(const CompilerInvocation &InitInvok,
CompilerInvocation Invocation(InitInvok);
auto FileSystem = llvm::vfs::getRealFileSystem();

IDEInspectionInstance IDEInspectionInst;
IDEInspectionInstance IDEInspectionInst(std::make_shared<PluginRegistry>());

std::unique_ptr<ide::OnDiskCodeCompletionCache> OnDiskCache;
if (!options::CompletionCachePath.empty())
Expand Down