From 757ee55ecdd3a64dae0db071e258d8f260722b26 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Tue, 30 Nov 2021 22:31:13 +0100 Subject: [PATCH 1/8] [SourceKit] Remove ability to specify fixed snapshots to use when building an AST The feature was never used and just complicated the implementation. --- .../lib/SwiftLang/SwiftASTManager.cpp | 82 ++++++------------- .../SourceKit/lib/SwiftLang/SwiftASTManager.h | 4 +- 2 files changed, 24 insertions(+), 62 deletions(-) diff --git a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp index c9c6c6693705d..cc40b4e172f37 100644 --- a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp +++ b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp @@ -372,22 +372,17 @@ class ASTBuildOperation /// Create a vector of text snapshots containing all files explicitly /// referenced by the compiler invocation and a vector of buffer stamps of - /// those files. For all files that have a snapshot specified in \p - /// FixedSnapshots, that snapshot is used. For all other files \p FileSystem - /// will be consulted for the current contents of the file. + /// those files. std::pair, std::vector> - snapshotAndStampsForFilesInCompilerInvocation( - ArrayRef FixedSnapshots); + snapshotAndStampsForFilesInCompilerInvocation(); public: ASTBuildOperation(IntrusiveRefCntPtr FileSystem, - ArrayRef Snapshots, SwiftInvocationRef InvokRef, SwiftASTManagerRef ASTManager, std::function DidFinishCallback) : InvokRef(InvokRef), FileSystem(FileSystem), ASTManager(ASTManager), DidFinishCallback(DidFinishCallback) { - auto SnapshotsAndStamps = - snapshotAndStampsForFilesInCompilerInvocation(Snapshots); + auto SnapshotsAndStamps = snapshotAndStampsForFilesInCompilerInvocation(); // const_cast is fine here. We just want to guard against modifying these // fields later on. It's fine to set them in the constructor. const_cast &>(this->Snapshots) = @@ -441,8 +436,7 @@ class ASTBuildOperation /// listed in the input files. As such, this might be \c true before the AST /// build and \c false after the AST has been built. See documentation on \c /// DependencyStamps for more info. - bool matchesSourceState(IntrusiveRefCntPtr fileSystem, - ArrayRef Snapshots); + bool matchesSourceState(IntrusiveRefCntPtr fileSystem); /// Called when a consumer is cancelled. This calls \c cancelled on the /// consumer, removes it from the \c Consumers severed by this build operation @@ -495,14 +489,12 @@ class ASTProducer : public std::enable_shared_from_this { /// Returns the latest build operation which can serve the \p Consumer or /// \c nullptr if no such build operation exists. - /// \p FileSystem and \p Snapshots describe the source state that the \p - /// Consumer expects. /// /// Assumes that \c BuildOperationsMtx has been claimed. ASTBuildOperationRef getBuildOperationForConsumer( SwiftASTConsumerRef Consumer, IntrusiveRefCntPtr FileSystem, - ArrayRef Snapshots, SwiftASTManagerRef Mgr); + SwiftASTManagerRef Mgr); public: explicit ASTProducer(SwiftInvocationRef InvokRef) @@ -513,7 +505,6 @@ class ASTProducer : public std::enable_shared_from_this { /// cancelled, \c failed or \c handlePrimaryAST callback. void enqueueConsumer(SwiftASTConsumerRef Consumer, IntrusiveRefCntPtr FileSystem, - ArrayRef Snapshots, SwiftASTManagerRef Mgr); size_t getMemoryCost() const { @@ -754,8 +745,7 @@ SwiftInvocationRef SwiftASTManager::getInvocation( void SwiftASTManager::processASTAsync( SwiftInvocationRef InvokRef, SwiftASTConsumerRef ASTConsumer, const void *OncePerASTToken, SourceKitCancellationToken CancellationToken, - llvm::IntrusiveRefCntPtr fileSystem, - ArrayRef Snapshots) { + llvm::IntrusiveRefCntPtr fileSystem) { assert(fileSystem); ASTProducerRef Producer = Impl.getASTProducer(InvokRef); @@ -775,8 +765,7 @@ void SwiftASTManager::processASTAsync( Impl.ScheduledConsumers.push_back({ASTConsumer, OncePerASTToken}); } - Producer->enqueueConsumer(ASTConsumer, fileSystem, Snapshots, - shared_from_this()); + Producer->enqueueConsumer(ASTConsumer, fileSystem, shared_from_this()); auto WeakConsumer = SwiftASTConsumerWeakRef(ASTConsumer); Impl.ReqTracker->setCancellationHandler(CancellationToken, [WeakConsumer] { @@ -860,26 +849,12 @@ SwiftASTManager::Implementation::getMemoryBuffer( return nullptr; } -/// If \p Snapshots contains a snapshot for the given \p Filename return it. -/// Otherwise, return \c None. -Optional -findSnapshot(ArrayRef Snapshots, - const std::string &Filename) { - for (auto &Snap : Snapshots) { - if (Snap->getFilename() == Filename) { - return Snap; - } - } - return None; -} - std::pair, std::vector> -ASTBuildOperation::snapshotAndStampsForFilesInCompilerInvocation( - ArrayRef FixedSnapshots) { +ASTBuildOperation::snapshotAndStampsForFilesInCompilerInvocation() { const InvocationOptions &Opts = InvokRef->Impl.Opts; std::string Error; // is ignored - std::vector Snapshots = FixedSnapshots.vec(); + std::vector Snapshots; std::vector Stamps; Stamps.reserve(Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); @@ -889,15 +864,11 @@ ASTBuildOperation::snapshotAndStampsForFilesInCompilerInvocation( Opts.Invok.getFrontendOptions().InputsAndOutputs.getAllInputs()) { const std::string &Filename = input.getFileName(); bool IsPrimary = input.isPrimary(); - if (auto FoundSnapshot = findSnapshot(Snapshots, Filename)) { - Stamps.push_back(FoundSnapshot.getValue()->getStamp()); - } else { - auto Content = ASTManager->Impl.getFileContent(Filename, IsPrimary, - FileSystem, Error); - Stamps.push_back(Content.Stamp); - if (Content.Snapshot) { - Snapshots.push_back(Content.Snapshot); - } + auto Content = + ASTManager->Impl.getFileContent(Filename, IsPrimary, FileSystem, Error); + Stamps.push_back(Content.Stamp); + if (Content.Snapshot) { + Snapshots.push_back(Content.Snapshot); } } assert(Stamps.size() == @@ -906,8 +877,7 @@ ASTBuildOperation::snapshotAndStampsForFilesInCompilerInvocation( } bool ASTBuildOperation::matchesSourceState( - llvm::IntrusiveRefCntPtr OtherFileSystem, - ArrayRef OtherSnapshots) { + llvm::IntrusiveRefCntPtr OtherFileSystem) { const SwiftInvocation::Implementation &Invok = InvokRef->Impl; // Check if the inputs changed. @@ -918,13 +888,8 @@ bool ASTBuildOperation::matchesSourceState( // snapshotAndStampsForFilesInCompilerInvocation. for (const auto &input : Invok.Opts.Invok.getFrontendOptions().InputsAndOutputs.getAllInputs()) { - const std::string &File = input.getFileName(); - if (auto FoundSnapshot = findSnapshot(OtherSnapshots, File)) { - InputStamps.push_back(FoundSnapshot.getValue()->getStamp()); - } else { - InputStamps.push_back( - ASTManager->Impl.getBufferStamp(File, OtherFileSystem)); - } + InputStamps.push_back( + ASTManager->Impl.getBufferStamp(input.getFileName(), OtherFileSystem)); } assert(InputStamps.size() == Invok.Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); @@ -1249,12 +1214,12 @@ bool ASTBuildOperation::addConsumer(SwiftASTConsumerRef Consumer) { ASTBuildOperationRef ASTProducer::getBuildOperationForConsumer( SwiftASTConsumerRef Consumer, IntrusiveRefCntPtr FileSystem, - ArrayRef Snapshots, SwiftASTManagerRef Mgr) { + SwiftASTManagerRef Mgr) { for (auto &BuildOp : llvm::reverse(BuildOperations)) { if (BuildOp->isCancelled()) { continue; } - if (BuildOp->matchesSourceState(FileSystem, Snapshots)) { + if (BuildOp->matchesSourceState(FileSystem)) { ++Mgr->Impl.Stats->numASTCacheHits; return BuildOp; } else if (Consumer->canUseASTWithSnapshots(BuildOp->getSnapshots())) { @@ -1268,19 +1233,18 @@ ASTBuildOperationRef ASTProducer::getBuildOperationForConsumer( void ASTProducer::enqueueConsumer( SwiftASTConsumerRef Consumer, IntrusiveRefCntPtr FileSystem, - ArrayRef Snapshots, SwiftASTManagerRef Mgr) { + SwiftASTManagerRef Mgr) { // We can't use a llvm::sys::ScopedLock here because we need to unlock it // before calling enqueueConsumer again in the !WasAdded case below. std::unique_lock BuildOperationsLock(BuildOperationsMtx); - if (auto BuildOp = - getBuildOperationForConsumer(Consumer, FileSystem, Snapshots, Mgr)) { + if (auto BuildOp = getBuildOperationForConsumer(Consumer, FileSystem, Mgr)) { bool WasAdded = BuildOp->addConsumer(Consumer); if (!WasAdded) { // The build operation was cancelled after the call to // getBuildOperationForConsumer but before the consumer could be added. // This should be an absolute edge case. Let's just try again. BuildOperationsLock.unlock(); - enqueueConsumer(Consumer, FileSystem, Snapshots, Mgr); + enqueueConsumer(Consumer, FileSystem, Mgr); return; } } else { @@ -1296,7 +1260,7 @@ void ASTProducer::enqueueConsumer( } }; ASTBuildOperationRef NewBuildOp = std::make_shared( - FileSystem, Snapshots, InvokRef, Mgr, DidFinishCallback); + FileSystem, InvokRef, Mgr, DidFinishCallback); BuildOperations.push_back(NewBuildOp); NewBuildOp->addConsumer(Consumer); NewBuildOp->schedule(Mgr->Impl.ASTBuildQueue); diff --git a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.h b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.h index a98349949ea58..6aafe9cb60193 100644 --- a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.h +++ b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.h @@ -248,9 +248,7 @@ class SwiftASTManager : public std::enable_shared_from_this { processASTAsync(SwiftInvocationRef Invok, SwiftASTConsumerRef ASTConsumer, const void *OncePerASTToken, SourceKitCancellationToken CancellationToken, - llvm::IntrusiveRefCntPtr fileSystem, - ArrayRef Snapshots = - ArrayRef()); + llvm::IntrusiveRefCntPtr fileSystem); std::unique_ptr getMemoryBuffer(StringRef Filename, std::string &Error); From eaf14ab6edd3ae3193a3abf3f3fdb3ac03c13d77 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Wed, 8 Dec 2021 15:16:15 +0100 Subject: [PATCH 2/8] [SourceKit] Store FileContents in ASTBuildOperation instead of snapshots --- .../lib/SwiftLang/SwiftASTManager.cpp | 137 ++++++++---------- 1 file changed, 58 insertions(+), 79 deletions(-) diff --git a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp index cc40b4e172f37..79e7e2c652c63 100644 --- a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp +++ b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp @@ -230,7 +230,7 @@ namespace { typedef uint64_t BufferStamp; -struct FileContent { +struct FileContent : llvm::RefCountedBase { ImmutableTextSnapshotRef Snapshot; std::string Filename; std::unique_ptr Buffer; @@ -248,6 +248,8 @@ struct FileContent { } }; +using FileContentRef = llvm::IntrusiveRefCntPtr; + /// An \c ASTBuildOperations builds an AST. Once the AST is built, it informs /// a list of \c SwiftASTConsumers about the built AST. /// It also supports cancellation with the following paradigm: If an \c @@ -299,7 +301,9 @@ class ASTBuildOperation /// Parameters necessary to build the AST. const SwiftInvocationRef InvokRef; const IntrusiveRefCntPtr FileSystem; - const std::vector Snapshots; + + /// The contents of all explicit input files of the compiler invoation. + const std::vector FileContents; /// Stamps of files used to build the AST. \c Stamps contains the stamps of /// all explicit input files, which can be determined at construction time of @@ -357,12 +361,6 @@ class ASTBuildOperation /// depends on the severity of the error. ASTUnitRef buildASTUnit(std::string &Error); - /// Retrieve the contents of all files needed for the compiler invocation to - /// build this AST. For files contained in \c Snapshots use the snapshot's - /// content. For all other files, consult the file system. - void findSnapshotAndOpenFiles(SmallVectorImpl &Contents, - std::string &Error) const; - /// Transition the build operation to \p NewState, asserting that the current /// state is \p ExpectedOldState. void transitionToState(State NewState, State ExpectedOldState) { @@ -370,11 +368,11 @@ class ASTBuildOperation OperationState = NewState; } - /// Create a vector of text snapshots containing all files explicitly + /// Create a vector of \c FileContents containing all files explicitly /// referenced by the compiler invocation and a vector of buffer stamps of /// those files. - std::pair, std::vector> - snapshotAndStampsForFilesInCompilerInvocation(); + std::pair, std::vector> + fileContentsAndStampsForFilesInCompilerInvocation(); public: ASTBuildOperation(IntrusiveRefCntPtr FileSystem, @@ -382,13 +380,14 @@ class ASTBuildOperation std::function DidFinishCallback) : InvokRef(InvokRef), FileSystem(FileSystem), ASTManager(ASTManager), DidFinishCallback(DidFinishCallback) { - auto SnapshotsAndStamps = snapshotAndStampsForFilesInCompilerInvocation(); + auto FileContentsAndStamps = + fileContentsAndStampsForFilesInCompilerInvocation(); // const_cast is fine here. We just want to guard against modifying these // fields later on. It's fine to set them in the constructor. - const_cast &>(this->Snapshots) = - SnapshotsAndStamps.first; + const_cast &>(this->FileContents) = + std::move(FileContentsAndStamps.first); const_cast &>(this->Stamps) = - SnapshotsAndStamps.second; + FileContentsAndStamps.second; } ~ASTBuildOperation() { @@ -398,7 +397,7 @@ class ASTBuildOperation "not receive their callback."); } - ArrayRef getSnapshots() const { return Snapshots; } + ArrayRef getFileContents() const { return FileContents; } /// Returns true if the build operation has finished. bool isFinished() { @@ -413,7 +412,7 @@ class ASTBuildOperation } size_t getMemoryCost() { - return sizeof(*this) + getVectorMemoryCost(Snapshots) + + return sizeof(*this) + getVectorMemoryCost(FileContents) + getVectorMemoryCost(Stamps) + Result.getMemoryCost(); } @@ -609,7 +608,7 @@ struct SwiftASTManager::Implementation { ASTProducerRef getASTProducer(SwiftInvocationRef InvokRef); - FileContent + FileContentRef getFileContent(StringRef FilePath, bool IsPrimary, IntrusiveRefCntPtr FileSystem, std::string &Error) const; @@ -644,10 +643,10 @@ SwiftASTManager::getMemoryBuffer(StringRef Filename, std::string &Error) { } static FrontendInputsAndOutputs -convertFileContentsToInputs(const SmallVectorImpl &contents) { +convertFileContentsToInputs(ArrayRef contents) { FrontendInputsAndOutputs inputsAndOutputs; - for (const FileContent &content : contents) - inputsAndOutputs.addInput(InputFile(content)); + for (FileContentRef content : contents) + inputsAndOutputs.addInput(InputFile(*content)); return inputsAndOutputs; } @@ -790,15 +789,16 @@ SwiftASTManager::Implementation::getASTProducer(SwiftInvocationRef InvokRef) { return Producer; } -static FileContent getFileContentFromSnap(ImmutableTextSnapshotRef Snap, - bool IsPrimary, StringRef FilePath) { +static FileContentRef getFileContentFromSnap(ImmutableTextSnapshotRef Snap, + bool IsPrimary, + StringRef FilePath) { auto Buf = llvm::MemoryBuffer::getMemBufferCopy( Snap->getBuffer()->getText(), FilePath); - return FileContent(Snap, FilePath.str(), std::move(Buf), IsPrimary, - Snap->getStamp()); + return new FileContent(Snap, FilePath.str(), std::move(Buf), IsPrimary, + Snap->getStamp()); } -FileContent SwiftASTManager::Implementation::getFileContent( +FileContentRef SwiftASTManager::Implementation::getFileContent( StringRef UnresolvedPath, bool IsPrimary, llvm::IntrusiveRefCntPtr FileSystem, std::string &Error) const { @@ -810,8 +810,8 @@ FileContent SwiftASTManager::Implementation::getFileContent( // FIXME: Is there a way to get timestamp and buffer for a file atomically ? auto Stamp = getBufferStamp(FilePath, FileSystem); auto Buffer = getMemoryBuffer(FilePath, FileSystem, Error); - return FileContent(nullptr, UnresolvedPath.str(), std::move(Buffer), - IsPrimary, Stamp); + return new FileContent(nullptr, UnresolvedPath.str(), std::move(Buffer), + IsPrimary, Stamp); } BufferStamp SwiftASTManager::Implementation::getBufferStamp( @@ -849,13 +849,15 @@ SwiftASTManager::Implementation::getMemoryBuffer( return nullptr; } -std::pair, std::vector> -ASTBuildOperation::snapshotAndStampsForFilesInCompilerInvocation() { +std::pair, std::vector> +ASTBuildOperation::fileContentsAndStampsForFilesInCompilerInvocation() { const InvocationOptions &Opts = InvokRef->Impl.Opts; std::string Error; // is ignored - std::vector Snapshots; + std::vector FileContents; std::vector Stamps; + FileContents.reserve( + Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); Stamps.reserve(Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); // IMPORTANT: The computation of stamps must match the one in @@ -866,33 +868,40 @@ ASTBuildOperation::snapshotAndStampsForFilesInCompilerInvocation() { bool IsPrimary = input.isPrimary(); auto Content = ASTManager->Impl.getFileContent(Filename, IsPrimary, FileSystem, Error); - Stamps.push_back(Content.Stamp); - if (Content.Snapshot) { - Snapshots.push_back(Content.Snapshot); + if (!Content->Buffer) { + LOG_WARN_FUNC("failed getting file contents for " << Filename << ": " + << Error); + // File may not exist, continue and recover as if it was empty. + Content->Buffer = + llvm::WritableMemoryBuffer::getNewMemBuffer(0, Filename); } + Stamps.push_back(Content->Stamp); + FileContents.push_back(Content); } assert(Stamps.size() == Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); - return std::make_pair(Snapshots, Stamps); + assert(FileContents.size() == + Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); + return std::make_pair(FileContents, Stamps); } bool ASTBuildOperation::matchesSourceState( llvm::IntrusiveRefCntPtr OtherFileSystem) { - const SwiftInvocation::Implementation &Invok = InvokRef->Impl; + const InvocationOptions &Opts = InvokRef->Impl.Opts; // Check if the inputs changed. std::vector InputStamps; InputStamps.reserve( - Invok.Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); + Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); // IMPORTANT: The computation of stamps must match the one in // snapshotAndStampsForFilesInCompilerInvocation. for (const auto &input : - Invok.Opts.Invok.getFrontendOptions().InputsAndOutputs.getAllInputs()) { + Opts.Invok.getFrontendOptions().InputsAndOutputs.getAllInputs()) { InputStamps.push_back( ASTManager->Impl.getBufferStamp(input.getFileName(), OtherFileSystem)); } assert(InputStamps.size() == - Invok.Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); + Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); if (Stamps != InputStamps) return false; @@ -1016,13 +1025,10 @@ ASTUnitRef ASTBuildOperation::buildASTUnit(std::string &Error) { Log->getOS() << Opts.Invok.getModuleName() << '/' << Opts.PrimaryFile; } - SmallVector Contents; - findSnapshotAndOpenFiles(Contents, Error); - ASTUnitRef ASTRef = new ASTUnit(++ASTUnitGeneration, ASTManager->Impl.Stats); - for (auto &Content : Contents) { - if (Content.Snapshot) - ASTRef->Impl.Snapshots.push_back(Content.Snapshot); + for (auto &Content : getFileContents()) { + if (Content->Snapshot) + ASTRef->Impl.Snapshots.push_back(Content->Snapshot); } auto &CompIns = ASTRef->Impl.CompInst; auto &Consumer = ASTRef->Impl.CollectDiagConsumer; @@ -1041,7 +1047,7 @@ ASTUnitRef ASTBuildOperation::buildASTUnit(std::string &Error) { CompilerInvocation Invocation; InvokRef->Impl.Opts.applyToSubstitutingInputs( - Invocation, convertFileContentsToInputs(Contents)); + Invocation, convertFileContentsToInputs(getFileContents())); Invocation.getLangOptions().CollectParsedToken = true; @@ -1111,38 +1117,6 @@ ASTUnitRef ASTBuildOperation::buildASTUnit(std::string &Error) { return ASTRef; } -void ASTBuildOperation::findSnapshotAndOpenFiles( - SmallVectorImpl &Contents, std::string &Error) const { - const InvocationOptions &Opts = InvokRef->Impl.Opts; - for (const auto &input : - Opts.Invok.getFrontendOptions().InputsAndOutputs.getAllInputs()) { - const std::string &File = input.getFileName(); - bool IsPrimary = input.isPrimary(); - bool FoundSnapshot = false; - for (auto &Snap : Snapshots) { - if (Snap->getFilename() == File) { - FoundSnapshot = true; - Contents.push_back(getFileContentFromSnap(Snap, IsPrimary, File)); - break; - } - } - if (FoundSnapshot) - continue; - - auto Content = - ASTManager->Impl.getFileContent(File, IsPrimary, FileSystem, Error); - if (!Content.Buffer) { - LOG_WARN_FUNC("failed getting file contents for " << File << ": " - << Error); - // File may not exist, continue and recover as if it was empty. - Content.Buffer = llvm::WritableMemoryBuffer::getNewMemBuffer(0, File); - } - Contents.push_back(std::move(Content)); - } - assert(Contents.size() == - Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); -} - void ASTBuildOperation::schedule(WorkQueue Queue) { transitionToState(State::Queued, /*ExpectedOldState=*/State::Created); auto SharedThis = shared_from_this(); @@ -1219,10 +1193,15 @@ ASTBuildOperationRef ASTProducer::getBuildOperationForConsumer( if (BuildOp->isCancelled()) { continue; } + std::vector Snapshots; + Snapshots.reserve(BuildOp->getFileContents().size()); + for (auto &FileContent : BuildOp->getFileContents()) { + Snapshots.push_back(FileContent->Snapshot); + } if (BuildOp->matchesSourceState(FileSystem)) { ++Mgr->Impl.Stats->numASTCacheHits; return BuildOp; - } else if (Consumer->canUseASTWithSnapshots(BuildOp->getSnapshots())) { + } else if (Consumer->canUseASTWithSnapshots(Snapshots)) { ++Mgr->Impl.Stats->numASTsUsedWithSnaphots; return BuildOp; } From cbf42eece021814d9972d8839e65df5629c1c87b Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Wed, 8 Dec 2021 15:16:29 +0100 Subject: [PATCH 3/8] [SourceKit] Don't store Stamps in ASTBuildOperation FileContents already contains the stamps. --- .../lib/SwiftLang/SwiftASTManager.cpp | 54 ++++++------------- 1 file changed, 16 insertions(+), 38 deletions(-) diff --git a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp index 79e7e2c652c63..8b781e0eaf259 100644 --- a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp +++ b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp @@ -302,14 +302,10 @@ class ASTBuildOperation const SwiftInvocationRef InvokRef; const IntrusiveRefCntPtr FileSystem; - /// The contents of all explicit input files of the compiler invoation. + /// The contents of all explicit input files of the compiler invoation, which + /// can be determined at construction time of the \c ASTBuildOperation. const std::vector FileContents; - /// Stamps of files used to build the AST. \c Stamps contains the stamps of - /// all explicit input files, which can be determined at construction time of - /// the \c ASTBuildOperation. - const std::vector Stamps; - /// \c DependencyStamps contains the stamps of all module depenecies needed /// for the AST build. These stamps are only known after the AST is built. /// Before the AST has been built, we thus assume that all dependency stamps @@ -369,10 +365,8 @@ class ASTBuildOperation } /// Create a vector of \c FileContents containing all files explicitly - /// referenced by the compiler invocation and a vector of buffer stamps of - /// those files. - std::pair, std::vector> - fileContentsAndStampsForFilesInCompilerInvocation(); + /// referenced by the compiler invocation. + std::vector fileContentsForFilesInCompilerInvocation(); public: ASTBuildOperation(IntrusiveRefCntPtr FileSystem, @@ -380,14 +374,10 @@ class ASTBuildOperation std::function DidFinishCallback) : InvokRef(InvokRef), FileSystem(FileSystem), ASTManager(ASTManager), DidFinishCallback(DidFinishCallback) { - auto FileContentsAndStamps = - fileContentsAndStampsForFilesInCompilerInvocation(); // const_cast is fine here. We just want to guard against modifying these // fields later on. It's fine to set them in the constructor. const_cast &>(this->FileContents) = - std::move(FileContentsAndStamps.first); - const_cast &>(this->Stamps) = - FileContentsAndStamps.second; + fileContentsForFilesInCompilerInvocation(); } ~ASTBuildOperation() { @@ -413,7 +403,7 @@ class ASTBuildOperation size_t getMemoryCost() { return sizeof(*this) + getVectorMemoryCost(FileContents) + - getVectorMemoryCost(Stamps) + Result.getMemoryCost(); + Result.getMemoryCost(); } /// Schedule building this AST on the given \p Queue. @@ -849,16 +839,14 @@ SwiftASTManager::Implementation::getMemoryBuffer( return nullptr; } -std::pair, std::vector> -ASTBuildOperation::fileContentsAndStampsForFilesInCompilerInvocation() { +std::vector +ASTBuildOperation::fileContentsForFilesInCompilerInvocation() { const InvocationOptions &Opts = InvokRef->Impl.Opts; std::string Error; // is ignored std::vector FileContents; - std::vector Stamps; FileContents.reserve( Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); - Stamps.reserve(Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); // IMPORTANT: The computation of stamps must match the one in // matchesSourceState. @@ -875,35 +863,25 @@ ASTBuildOperation::fileContentsAndStampsForFilesInCompilerInvocation() { Content->Buffer = llvm::WritableMemoryBuffer::getNewMemBuffer(0, Filename); } - Stamps.push_back(Content->Stamp); FileContents.push_back(Content); } - assert(Stamps.size() == - Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); assert(FileContents.size() == Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); - return std::make_pair(FileContents, Stamps); + return FileContents; } bool ASTBuildOperation::matchesSourceState( llvm::IntrusiveRefCntPtr OtherFileSystem) { const InvocationOptions &Opts = InvokRef->Impl.Opts; - // Check if the inputs changed. - std::vector InputStamps; - InputStamps.reserve( - Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); - // IMPORTANT: The computation of stamps must match the one in - // snapshotAndStampsForFilesInCompilerInvocation. - for (const auto &input : - Opts.Invok.getFrontendOptions().InputsAndOutputs.getAllInputs()) { - InputStamps.push_back( - ASTManager->Impl.getBufferStamp(input.getFileName(), OtherFileSystem)); + auto Inputs = Opts.Invok.getFrontendOptions().InputsAndOutputs.getAllInputs(); + for (size_t I = 0; I < Inputs.size(); I++) { + if (getFileContents()[I]->Stamp != + ASTManager->Impl.getBufferStamp(Inputs[I].getFileName(), + OtherFileSystem)) { + return false; + } } - assert(InputStamps.size() == - Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); - if (Stamps != InputStamps) - return false; for (auto &Dependency : DependencyStamps) { if (Dependency.second != From fd9b4efbe121077cb469a9ea7401062513c79419 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Tue, 30 Nov 2021 22:30:12 +0100 Subject: [PATCH 4/8] [SourceKit] Remove some dupliate computation of realpaths --- .../lib/SwiftLang/SwiftASTManager.cpp | 18 ++++++++++++------ tools/SourceKit/lib/SwiftLang/SwiftEditor.cpp | 10 +++++++--- .../SourceKit/lib/SwiftLang/SwiftLangSupport.h | 5 ++++- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp index 8b781e0eaf259..ede67d77400a1 100644 --- a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp +++ b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp @@ -605,7 +605,8 @@ struct SwiftASTManager::Implementation { BufferStamp getBufferStamp(StringRef FilePath, - IntrusiveRefCntPtr FileSystem) const; + IntrusiveRefCntPtr FileSystem, + bool CheckEditorDocs = true) const; std::unique_ptr getMemoryBuffer(StringRef Filename, @@ -793,12 +794,13 @@ FileContentRef SwiftASTManager::Implementation::getFileContent( llvm::IntrusiveRefCntPtr FileSystem, std::string &Error) const { std::string FilePath = SwiftLangSupport::resolvePathSymlinks(UnresolvedPath); - if (auto EditorDoc = EditorDocs->findByPath(FilePath)) + if (auto EditorDoc = EditorDocs->findByPath(FilePath, /*IsRealpath=*/true)) return getFileContentFromSnap(EditorDoc->getLatestSnapshot(), IsPrimary, FilePath); // FIXME: Is there a way to get timestamp and buffer for a file atomically ? - auto Stamp = getBufferStamp(FilePath, FileSystem); + // No need to check EditorDocs again. We did so above. + auto Stamp = getBufferStamp(FilePath, FileSystem, /*CheckEditorDocs=*/false); auto Buffer = getMemoryBuffer(FilePath, FileSystem, Error); return new FileContent(nullptr, UnresolvedPath.str(), std::move(Buffer), IsPrimary, Stamp); @@ -806,11 +808,15 @@ FileContentRef SwiftASTManager::Implementation::getFileContent( BufferStamp SwiftASTManager::Implementation::getBufferStamp( StringRef FilePath, - llvm::IntrusiveRefCntPtr FileSystem) const { + llvm::IntrusiveRefCntPtr FileSystem, + bool CheckEditorDocs) const { assert(FileSystem); - if (auto EditorDoc = EditorDocs->findByPath(FilePath)) - return EditorDoc->getLatestSnapshot()->getStamp(); + if (CheckEditorDocs) { + if (auto EditorDoc = EditorDocs->findByPath(FilePath)) { + return EditorDoc->getLatestSnapshot()->getStamp(); + } + } auto StatusOrErr = FileSystem->status(FilePath); if (std::error_code Err = StatusOrErr.getError()) { diff --git a/tools/SourceKit/lib/SwiftLang/SwiftEditor.cpp b/tools/SourceKit/lib/SwiftLang/SwiftEditor.cpp index f050bfe53a9e9..0cbba7d5575a5 100644 --- a/tools/SourceKit/lib/SwiftLang/SwiftEditor.cpp +++ b/tools/SourceKit/lib/SwiftLang/SwiftEditor.cpp @@ -245,14 +245,18 @@ SwiftEditorDocumentFileMap::getByUnresolvedName(StringRef FilePath) { } SwiftEditorDocumentRef -SwiftEditorDocumentFileMap::findByPath(StringRef FilePath) { +SwiftEditorDocumentFileMap::findByPath(StringRef FilePath, bool IsRealpath) { SwiftEditorDocumentRef EditorDoc; - std::string ResolvedPath = SwiftLangSupport::resolvePathSymlinks(FilePath); + std::string Scratch; + if (!IsRealpath) { + Scratch = SwiftLangSupport::resolvePathSymlinks(FilePath); + FilePath = Scratch; + } Queue.dispatchSync([&]{ for (auto &Entry : Docs) { if (Entry.getKey() == FilePath || - Entry.getValue().ResolvedPath == ResolvedPath) { + Entry.getValue().ResolvedPath == FilePath) { EditorDoc = Entry.getValue().DocRef; break; } diff --git a/tools/SourceKit/lib/SwiftLang/SwiftLangSupport.h b/tools/SourceKit/lib/SwiftLang/SwiftLangSupport.h index 03b9fb747d270..ddaceafe41530 100644 --- a/tools/SourceKit/lib/SwiftLang/SwiftLangSupport.h +++ b/tools/SourceKit/lib/SwiftLang/SwiftLangSupport.h @@ -156,7 +156,10 @@ class SwiftEditorDocumentFileMap { /// Looks up the document only by the path name that was given initially. SwiftEditorDocumentRef getByUnresolvedName(StringRef FilePath); /// Looks up the document by resolving symlinks in the paths. - SwiftEditorDocumentRef findByPath(StringRef FilePath); + /// If \p IsRealpath is \c true, then \p FilePath must already be + /// canonicalized to a realpath. + SwiftEditorDocumentRef findByPath(StringRef FilePath, + bool IsRealpath = false); SwiftEditorDocumentRef remove(StringRef FilePath); }; From 920878e6acfe00e9db4d716452df5d732012c417 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Wed, 8 Dec 2021 15:16:39 +0100 Subject: [PATCH 5/8] [SourceKit] Make FileContent non ref-counted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We never need to have two copies of the same `FileContent` object, so we don’t need a copy constructor and can thus pass it on the stack again, instead of storing it on the heap. --- .../lib/SwiftLang/SwiftASTManager.cpp | 54 +++++++++---------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp index ede67d77400a1..601aa55d75f15 100644 --- a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp +++ b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp @@ -230,7 +230,7 @@ namespace { typedef uint64_t BufferStamp; -struct FileContent : llvm::RefCountedBase { +struct FileContent { ImmutableTextSnapshotRef Snapshot; std::string Filename; std::unique_ptr Buffer; @@ -248,8 +248,6 @@ struct FileContent : llvm::RefCountedBase { } }; -using FileContentRef = llvm::IntrusiveRefCntPtr; - /// An \c ASTBuildOperations builds an AST. Once the AST is built, it informs /// a list of \c SwiftASTConsumers about the built AST. /// It also supports cancellation with the following paradigm: If an \c @@ -304,7 +302,7 @@ class ASTBuildOperation /// The contents of all explicit input files of the compiler invoation, which /// can be determined at construction time of the \c ASTBuildOperation. - const std::vector FileContents; + const std::vector FileContents; /// \c DependencyStamps contains the stamps of all module depenecies needed /// for the AST build. These stamps are only known after the AST is built. @@ -366,7 +364,7 @@ class ASTBuildOperation /// Create a vector of \c FileContents containing all files explicitly /// referenced by the compiler invocation. - std::vector fileContentsForFilesInCompilerInvocation(); + std::vector fileContentsForFilesInCompilerInvocation(); public: ASTBuildOperation(IntrusiveRefCntPtr FileSystem, @@ -376,7 +374,7 @@ class ASTBuildOperation DidFinishCallback(DidFinishCallback) { // const_cast is fine here. We just want to guard against modifying these // fields later on. It's fine to set them in the constructor. - const_cast &>(this->FileContents) = + const_cast &>(this->FileContents) = fileContentsForFilesInCompilerInvocation(); } @@ -387,7 +385,7 @@ class ASTBuildOperation "not receive their callback."); } - ArrayRef getFileContents() const { return FileContents; } + ArrayRef getFileContents() const { return FileContents; } /// Returns true if the build operation has finished. bool isFinished() { @@ -598,7 +596,7 @@ struct SwiftASTManager::Implementation { ASTProducerRef getASTProducer(SwiftInvocationRef InvokRef); - FileContentRef + FileContent getFileContent(StringRef FilePath, bool IsPrimary, IntrusiveRefCntPtr FileSystem, std::string &Error) const; @@ -634,10 +632,10 @@ SwiftASTManager::getMemoryBuffer(StringRef Filename, std::string &Error) { } static FrontendInputsAndOutputs -convertFileContentsToInputs(ArrayRef contents) { +convertFileContentsToInputs(ArrayRef contents) { FrontendInputsAndOutputs inputsAndOutputs; - for (FileContentRef content : contents) - inputsAndOutputs.addInput(InputFile(*content)); + for (const FileContent &content : contents) + inputsAndOutputs.addInput(InputFile(content)); return inputsAndOutputs; } @@ -780,16 +778,15 @@ SwiftASTManager::Implementation::getASTProducer(SwiftInvocationRef InvokRef) { return Producer; } -static FileContentRef getFileContentFromSnap(ImmutableTextSnapshotRef Snap, - bool IsPrimary, - StringRef FilePath) { +static FileContent getFileContentFromSnap(ImmutableTextSnapshotRef Snap, + bool IsPrimary, StringRef FilePath) { auto Buf = llvm::MemoryBuffer::getMemBufferCopy( Snap->getBuffer()->getText(), FilePath); - return new FileContent(Snap, FilePath.str(), std::move(Buf), IsPrimary, - Snap->getStamp()); + return FileContent(Snap, FilePath.str(), std::move(Buf), IsPrimary, + Snap->getStamp()); } -FileContentRef SwiftASTManager::Implementation::getFileContent( +FileContent SwiftASTManager::Implementation::getFileContent( StringRef UnresolvedPath, bool IsPrimary, llvm::IntrusiveRefCntPtr FileSystem, std::string &Error) const { @@ -802,8 +799,8 @@ FileContentRef SwiftASTManager::Implementation::getFileContent( // No need to check EditorDocs again. We did so above. auto Stamp = getBufferStamp(FilePath, FileSystem, /*CheckEditorDocs=*/false); auto Buffer = getMemoryBuffer(FilePath, FileSystem, Error); - return new FileContent(nullptr, UnresolvedPath.str(), std::move(Buffer), - IsPrimary, Stamp); + return FileContent(nullptr, UnresolvedPath.str(), std::move(Buffer), + IsPrimary, Stamp); } BufferStamp SwiftASTManager::Implementation::getBufferStamp( @@ -845,12 +842,12 @@ SwiftASTManager::Implementation::getMemoryBuffer( return nullptr; } -std::vector +std::vector ASTBuildOperation::fileContentsForFilesInCompilerInvocation() { const InvocationOptions &Opts = InvokRef->Impl.Opts; std::string Error; // is ignored - std::vector FileContents; + std::vector FileContents; FileContents.reserve( Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); @@ -862,14 +859,13 @@ ASTBuildOperation::fileContentsForFilesInCompilerInvocation() { bool IsPrimary = input.isPrimary(); auto Content = ASTManager->Impl.getFileContent(Filename, IsPrimary, FileSystem, Error); - if (!Content->Buffer) { + if (!Content.Buffer) { LOG_WARN_FUNC("failed getting file contents for " << Filename << ": " << Error); // File may not exist, continue and recover as if it was empty. - Content->Buffer = - llvm::WritableMemoryBuffer::getNewMemBuffer(0, Filename); + Content.Buffer = llvm::WritableMemoryBuffer::getNewMemBuffer(0, Filename); } - FileContents.push_back(Content); + FileContents.push_back(std::move(Content)); } assert(FileContents.size() == Opts.Invok.getFrontendOptions().InputsAndOutputs.inputCount()); @@ -882,7 +878,7 @@ bool ASTBuildOperation::matchesSourceState( auto Inputs = Opts.Invok.getFrontendOptions().InputsAndOutputs.getAllInputs(); for (size_t I = 0; I < Inputs.size(); I++) { - if (getFileContents()[I]->Stamp != + if (getFileContents()[I].Stamp != ASTManager->Impl.getBufferStamp(Inputs[I].getFileName(), OtherFileSystem)) { return false; @@ -1011,8 +1007,8 @@ ASTUnitRef ASTBuildOperation::buildASTUnit(std::string &Error) { ASTUnitRef ASTRef = new ASTUnit(++ASTUnitGeneration, ASTManager->Impl.Stats); for (auto &Content : getFileContents()) { - if (Content->Snapshot) - ASTRef->Impl.Snapshots.push_back(Content->Snapshot); + if (Content.Snapshot) + ASTRef->Impl.Snapshots.push_back(Content.Snapshot); } auto &CompIns = ASTRef->Impl.CompInst; auto &Consumer = ASTRef->Impl.CollectDiagConsumer; @@ -1180,7 +1176,7 @@ ASTBuildOperationRef ASTProducer::getBuildOperationForConsumer( std::vector Snapshots; Snapshots.reserve(BuildOp->getFileContents().size()); for (auto &FileContent : BuildOp->getFileContents()) { - Snapshots.push_back(FileContent->Snapshot); + Snapshots.push_back(FileContent.Snapshot); } if (BuildOp->matchesSourceState(FileSystem)) { ++Mgr->Impl.Stats->numASTCacheHits; From 1ec362c1de569b24f3a1610c392b42879c9a4aae Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Wed, 8 Dec 2021 15:13:01 +0100 Subject: [PATCH 6/8] [SourceKit] Improve memory cost function of ASTBuildOperation --- tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp index 601aa55d75f15..e37a3ad13b455 100644 --- a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp +++ b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp @@ -246,6 +246,10 @@ struct FileContent { explicit operator InputFile() const { return InputFile(Filename, IsPrimary, Buffer.get()); } + + size_t getMemoryCost() const { + return sizeof(*this) + Filename.size() + Buffer->getBufferSize(); + } }; /// An \c ASTBuildOperations builds an AST. Once the AST is built, it informs @@ -400,8 +404,12 @@ class ASTBuildOperation } size_t getMemoryCost() { - return sizeof(*this) + getVectorMemoryCost(FileContents) + - Result.getMemoryCost(); + size_t Cost = sizeof(*this) + getVectorMemoryCost(FileContents) + + Result.getMemoryCost(); + for (const FileContent &File : FileContents) { + Cost += File.getMemoryCost(); + } + return Cost; } /// Schedule building this AST on the given \p Queue. From b9379aea293f645c1db3e82e74aa82d63c79e87c Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Thu, 9 Dec 2021 22:58:56 +0100 Subject: [PATCH 7/8] [SourceKit] Enqueue `SwiftASTConsumer`s async on a queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enqueuing `SwiftASTConsumer`s might be expensive because `getBuildOperationForConsumer` consults the file system. Since all results from the AST build are processed asynchronously anyway, there’s no need to perform the enqueuing synchronously. rdar://86289703 --- .../lib/SwiftLang/SwiftASTManager.cpp | 74 ++++++++++--------- .../SourceKit/lib/SwiftLang/SwiftASTManager.h | 11 ++- 2 files changed, 50 insertions(+), 35 deletions(-) diff --git a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp index e37a3ad13b455..1cbc21a533baf 100644 --- a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp +++ b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp @@ -457,7 +457,8 @@ class ASTProducer : public std::enable_shared_from_this { /// execute. Operations are guaranteed to be in FIFO order, that is the first /// one in the vector is the oldes build operation. SmallVector BuildOperations = {}; - llvm::sys::Mutex BuildOperationsMtx; + WorkQueue BuildOperationsQueue = WorkQueue( + WorkQueue::Dequeuing::Serial, "ASTProducer.BuildOperationsQueue"); /// Erase all finished build operations with a result except for the latest /// one which contains a successful results. @@ -465,7 +466,7 @@ class ASTProducer : public std::enable_shared_from_this { /// but keeps the latest AST around, so that new consumers can be served from /// it, if possible. /// - /// Assumes that \c BuildOperationsMtx has been claimed. + /// Must be executed on \c BuildOperationsQueue. void cleanBuildOperations() { auto ReverseOperations = llvm::reverse(BuildOperations); auto LastOperationWithResultIt = @@ -485,7 +486,7 @@ class ASTProducer : public std::enable_shared_from_this { /// Returns the latest build operation which can serve the \p Consumer or /// \c nullptr if no such build operation exists. /// - /// Assumes that \c BuildOperationsMtx has been claimed. + /// Must be executed on \c BuildOperationsQueue. ASTBuildOperationRef getBuildOperationForConsumer( SwiftASTConsumerRef Consumer, IntrusiveRefCntPtr FileSystem, @@ -1162,13 +1163,13 @@ bool ASTBuildOperation::addConsumer(SwiftASTConsumerRef Consumer) { } else { assert(OperationState != State::Finished); auto WeakThis = std::weak_ptr(shared_from_this()); + Consumers.push_back(Consumer); Consumer->setCancellationRequestCallback( [WeakThis](SwiftASTConsumerRef Consumer) { if (auto This = WeakThis.lock()) { This->requestConsumerCancellation(Consumer); } }); - Consumers.push_back(Consumer); } return true; } @@ -1201,35 +1202,40 @@ void ASTProducer::enqueueConsumer( SwiftASTConsumerRef Consumer, IntrusiveRefCntPtr FileSystem, SwiftASTManagerRef Mgr) { - // We can't use a llvm::sys::ScopedLock here because we need to unlock it - // before calling enqueueConsumer again in the !WasAdded case below. - std::unique_lock BuildOperationsLock(BuildOperationsMtx); - if (auto BuildOp = getBuildOperationForConsumer(Consumer, FileSystem, Mgr)) { - bool WasAdded = BuildOp->addConsumer(Consumer); - if (!WasAdded) { - // The build operation was cancelled after the call to - // getBuildOperationForConsumer but before the consumer could be added. - // This should be an absolute edge case. Let's just try again. - BuildOperationsLock.unlock(); - enqueueConsumer(Consumer, FileSystem, Mgr); - return; - } - } else { - auto WeakThis = std::weak_ptr(shared_from_this()); - auto DidFinishCallback = [WeakThis, Mgr]() { - if (auto This = WeakThis.lock()) { - { - llvm::sys::ScopedLock L(This->BuildOperationsMtx); - This->cleanBuildOperations(); - } - // Re-register the object with the cache to update its memory cost. - Mgr->Impl.ASTCache.set(This->InvokRef->Impl.Key, This); + // Enqueue the consumer in the background because getBuildOperationForConsumer + // consults the file system and might be slow. Also, there's no need to do + // this synchronously since all results will be delivered async anyway. + auto This = shared_from_this(); + BuildOperationsQueue.dispatch([Consumer, FileSystem, Mgr, This]() { + if (auto BuildOp = + This->getBuildOperationForConsumer(Consumer, FileSystem, Mgr)) { + bool WasAdded = BuildOp->addConsumer(Consumer); + if (!WasAdded) { + // The build operation was cancelled after the call to + // getBuildOperationForConsumer but before the consumer could be + // added. This should be an absolute edge case. Let's just try + // again. + This->enqueueConsumer(Consumer, FileSystem, Mgr); } - }; - ASTBuildOperationRef NewBuildOp = std::make_shared( - FileSystem, InvokRef, Mgr, DidFinishCallback); - BuildOperations.push_back(NewBuildOp); - NewBuildOp->addConsumer(Consumer); - NewBuildOp->schedule(Mgr->Impl.ASTBuildQueue); - } + } else { + auto WeakThis = std::weak_ptr(This); + auto DidFinishCallback = [WeakThis, Mgr]() { + if (auto This = WeakThis.lock()) { + This->BuildOperationsQueue.dispatchSync( + [This]() { This->cleanBuildOperations(); }); + // Re-register the object with the cache to update its memory + // cost. + Mgr->Impl.ASTCache.set(This->InvokRef->Impl.Key, This); + } + }; + ASTBuildOperationRef NewBuildOp = std::make_shared( + FileSystem, This->InvokRef, Mgr, DidFinishCallback); + This->BuildOperations.push_back(NewBuildOp); + bool WasAdded = NewBuildOp->addConsumer(Consumer); + assert(WasAdded && "Consumer wasn't added to a new build operation " + "that can't have been cancelled yet?"); + (void)WasAdded; + NewBuildOp->schedule(Mgr->Impl.ASTBuildQueue); + } + }); } diff --git a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.h b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.h index 6aafe9cb60193..6504a87a49507 100644 --- a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.h +++ b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.h @@ -145,6 +145,8 @@ class SwiftASTConsumer : public std::enable_shared_from_this { Optional)>> CancellationRequestCallback; + bool IsCancelled = false; + public: virtual ~SwiftASTConsumer() { } @@ -157,6 +159,7 @@ class SwiftASTConsumer : public std::enable_shared_from_this { /// depending on it. void requestCancellation() { llvm::sys::ScopedLock L(CancellationRequestCallbackMtx); + IsCancelled = true; if (CancellationRequestCallback.hasValue()) { (*CancellationRequestCallback)(shared_from_this()); CancellationRequestCallback = None; @@ -168,12 +171,18 @@ class SwiftASTConsumer : public std::enable_shared_from_this { /// currently no callback set. /// The cancellation request callback will automatically be removed when the /// SwiftASTManager is cancelled. + /// If this \c SwiftASTConsumer has already been cancelled when this method is + /// called, \c NewCallback will be called immediately. void setCancellationRequestCallback( std::function)> NewCallback) { llvm::sys::ScopedLock L(CancellationRequestCallbackMtx); assert(!CancellationRequestCallback.hasValue() && "Can't set two cancellation callbacks on a SwiftASTConsumer"); - CancellationRequestCallback = NewCallback; + if (IsCancelled) { + NewCallback(shared_from_this()); + } else { + CancellationRequestCallback = NewCallback; + } } /// Removes the cancellation request callback previously set by \c From 2e9bf34a254b12c31e1f2191a41fcf6e70cbf0c2 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Fri, 10 Dec 2021 16:44:10 +0100 Subject: [PATCH 8/8] [SourceKit] Fix a crash that occurred when a document without an associated source file is reopened MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If a file isn’t opened with a `key.sourcefile` argument, we don’t store a snapshot for it. This could cause a crash when trying to consult its snapshot to see whether an AST can be reused for cursor info. --- ...o-crash-reopen-without-compiler-args.swift | 51 +++++++++++++++++++ .../lib/SwiftLang/SwiftASTManager.cpp | 4 +- 2 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 test/SourceKit/Misc/no-crash-reopen-without-compiler-args.swift diff --git a/test/SourceKit/Misc/no-crash-reopen-without-compiler-args.swift b/test/SourceKit/Misc/no-crash-reopen-without-compiler-args.swift new file mode 100644 index 0000000000000..f7624b979d69f --- /dev/null +++ b/test/SourceKit/Misc/no-crash-reopen-without-compiler-args.swift @@ -0,0 +1,51 @@ +// RUN: %empty-directory(%t) +// RUN: %{python} %utils/split_file.py -o %t %s +// RUN: %sourcekitd-test \ +// RUN: -json-request-path %t/1-open-without-sourcefile.json \ +// RUN: == -json-request-path %t/2-close.json \ +// RUN: == -json-request-path %t/3-reopen-without-compiler-args.json \ +// RUN: == -json-request-path %t/4-cursor-info.json + +// This used to crash with a nullptr dereference because we didn't store a +// snapshot in the FileContents of a.swift since it wasn't opened with a +// key.sourcefile argument. + +// BEGIN 1-open-without-sourcefile.json +{ + key.request: source.request.editor.open, + key.name: "/invalid/a.swift", + key.compilerargs: [ + "/invalid/a.swift", + "/invalid/b.swift" + ], + key.sourcetext: "", + key.enablesyntaxmap: 0, + key.enablesubstructure: 0, + key.enablediagnostics: 0 +} +// BEGIN 2-close.json +{ + key.request: source.request.editor.close, + key.name: "/invalid/a.swift" +} +// BEGIN 3-reopen-without-compiler-args.json +{ + key.request: source.request.editor.open, + key.name: "/invalid/a.swift", + key.compilerargs: [ + ], + key.sourcetext: "", + key.enablesyntaxmap: 0, + key.enablesubstructure: 0, + key.enablediagnostics: 0 +} +// BEGIN 4-cursor-info.json +{ + key.request: source.request.cursorinfo, + key.compilerargs: [ + "/invalid/a.swift", + "/invalid/b.swift" + ], + key.offset: 0, + key.sourcefile: "/invalid/a.swift" +} diff --git a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp index 1cbc21a533baf..954cb43d481d2 100644 --- a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp +++ b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp @@ -1185,7 +1185,9 @@ ASTBuildOperationRef ASTProducer::getBuildOperationForConsumer( std::vector Snapshots; Snapshots.reserve(BuildOp->getFileContents().size()); for (auto &FileContent : BuildOp->getFileContents()) { - Snapshots.push_back(FileContent.Snapshot); + if (FileContent.Snapshot) { + Snapshots.push_back(FileContent.Snapshot); + } } if (BuildOp->matchesSourceState(FileSystem)) { ++Mgr->Impl.Stats->numASTCacheHits;