Skip to content

Commit 4d956af

Browse files
sam-mccallsam-mccall
committed
Revert [clangd] Extract per-dir CDB cache to its own threadsafe class. NFC
This reverts commit 8a4390d. (The reland did not have the bugfix, just trying to get more details from the buildbots)
1 parent 8a4390d commit 4d956af

File tree

2 files changed

+88
-194
lines changed

2 files changed

+88
-194
lines changed

‎clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

Lines changed: 72 additions & 180 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,11 @@
1616
#include "llvm/ADT/None.h"
1717
#include "llvm/ADT/Optional.h"
1818
#include "llvm/ADT/STLExtras.h"
19-
#include "llvm/ADT/ScopeExit.h"
2019
#include "llvm/ADT/SmallString.h"
2120
#include "llvm/Support/FileSystem.h"
2221
#include "llvm/Support/FileUtilities.h"
2322
#include "llvm/Support/Path.h"
2423
#include "llvm/Support/Program.h"
25-
#include <chrono>
2624
#include <string>
2725
#include <tuple>
2826
#include <vector>
@@ -60,117 +58,10 @@ GlobalCompilationDatabase::getFallbackCommand(PathRef File) const {
6058
return Cmd;
6159
}
6260

63-
// Loads and caches the CDB from a single directory.
64-
//
65-
// This class is threadsafe, which is to say we have independent locks for each
66-
// directory we're searching for a CDB.
67-
// Loading is deferred until first access.
68-
//
69-
// The DirectoryBasedCDB keeps a map from path => DirectoryCache.
70-
// Typical usage is to:
71-
// - 1) determine all the paths that might be searched
72-
// - 2) acquire the map lock and get-or-create all the DirectoryCache entries
73-
// - 3) release the map lock and query the caches as desired
74-
//
75-
// FIXME: this should revalidate the cache sometimes
76-
// FIXME: IO should go through a VFS
77-
class DirectoryBasedGlobalCompilationDatabase::DirectoryCache {
78-
// Absolute canonical path that we're the cache for. (Not case-folded).
79-
const std::string Path;
80-
81-
// True if we've looked for a CDB here and found none.
82-
// (This makes it possible for get() to return without taking a lock)
83-
// FIXME: this should have an expiry time instead of lasting forever.
84-
std::atomic<bool> FinalizedNoCDB = {false};
85-
86-
// Guards following cache state.
87-
std::mutex Mu;
88-
// Has cache been filled from disk? FIXME: this should be an expiry time.
89-
bool CachePopulated = false;
90-
// Whether a new CDB has been loaded but not broadcast yet.
91-
bool NeedsBroadcast = false;
92-
// Last loaded CDB, meaningful if CachePopulated is set.
93-
// shared_ptr so we can overwrite this when callers are still using the CDB.
94-
std::shared_ptr<tooling::CompilationDatabase> CDB;
95-
96-
public:
97-
DirectoryCache(llvm::StringRef Path) : Path(Path) {
98-
assert(llvm::sys::path::is_absolute(Path));
99-
}
100-
101-
// Get the CDB associated with this directory.
102-
// ShouldBroadcast:
103-
// - as input, signals whether the caller is willing to broadcast a
104-
// newly-discovered CDB. (e.g. to trigger background indexing)
105-
// - as output, signals whether the caller should do so.
106-
// (If a new CDB is discovered and ShouldBroadcast is false, we mark the
107-
// CDB as needing broadcast, and broadcast it next time we can).
108-
std::shared_ptr<const tooling::CompilationDatabase>
109-
get(bool &ShouldBroadcast) {
110-
// Fast path for common case without taking lock.
111-
if (FinalizedNoCDB.load()) {
112-
ShouldBroadcast = false;
113-
return nullptr;
114-
}
115-
std::lock_guard<std::mutex> Lock(Mu);
116-
auto RequestBroadcast = llvm::make_scope_exit([&, OldCDB(CDB.get())] {
117-
// If we loaded a new CDB, it should be broadcast at some point.
118-
if (CDB != nullptr && CDB.get() != OldCDB)
119-
NeedsBroadcast = true;
120-
else if (CDB == nullptr) // nothing to broadcast anymore!
121-
NeedsBroadcast = false;
122-
// If we have something to broadcast, then do so iff allowed.
123-
if (!ShouldBroadcast)
124-
return;
125-
ShouldBroadcast = NeedsBroadcast;
126-
NeedsBroadcast = false;
127-
});
128-
129-
// For now, we never actually attempt to revalidate a populated cache.
130-
if (CachePopulated)
131-
return CDB;
132-
assert(CDB == nullptr);
133-
134-
load();
135-
CachePopulated = true;
136-
137-
if (!CDB)
138-
FinalizedNoCDB.store(true);
139-
return CDB;
140-
}
141-
142-
llvm::StringRef path() const { return Path; }
143-
144-
private:
145-
// Updates `CDB` from disk state.
146-
void load() {
147-
std::string Error; // ignored, because it's often "didn't find anything".
148-
CDB = tooling::CompilationDatabase::loadFromDirectory(Path, Error);
149-
if (!CDB) {
150-
// Fallback: check for $src/build, the conventional CMake build root.
151-
// Probe existence first to avoid each plugin doing IO if it doesn't
152-
// exist.
153-
llvm::SmallString<256> BuildDir(Path);
154-
llvm::sys::path::append(BuildDir, "build");
155-
if (llvm::sys::fs::is_directory(BuildDir)) {
156-
vlog("Found candidate build directory {0}", BuildDir);
157-
CDB = tooling::CompilationDatabase::loadFromDirectory(BuildDir, Error);
158-
}
159-
}
160-
if (CDB) {
161-
log("Loaded compilation database from {0}", Path);
162-
} else {
163-
vlog("No compilation database at {0}", Path);
164-
}
165-
}
166-
};
167-
16861
DirectoryBasedGlobalCompilationDatabase::
16962
DirectoryBasedGlobalCompilationDatabase(
170-
llvm::Optional<Path> CompileCommandsDir) {
171-
if (CompileCommandsDir)
172-
OnlyDirCache = std::make_unique<DirectoryCache>(*CompileCommandsDir);
173-
}
63+
llvm::Optional<Path> CompileCommandsDir)
64+
: CompileCommandsDir(std::move(CompileCommandsDir)) {}
17465

17566
DirectoryBasedGlobalCompilationDatabase::
17667
~DirectoryBasedGlobalCompilationDatabase() = default;
@@ -216,26 +107,31 @@ static bool pathEqual(PathRef A, PathRef B) {
216107
#endif
217108
}
218109

219-
std::vector<DirectoryBasedGlobalCompilationDatabase::DirectoryCache *>
220-
DirectoryBasedGlobalCompilationDatabase::getDirectoryCaches(
221-
llvm::ArrayRef<llvm::StringRef> Dirs) const {
222-
std::vector<std::string> FoldedDirs;
223-
FoldedDirs.reserve(Dirs.size());
224-
for (const auto &Dir : Dirs) {
225-
#ifndef NDEBUG
226-
if (!llvm::sys::path::is_absolute(Dir))
227-
elog("Trying to cache CDB for relative {0}");
228-
#endif
229-
FoldedDirs.push_back(maybeCaseFoldPath(Dir));
110+
DirectoryBasedGlobalCompilationDatabase::CachedCDB &
111+
DirectoryBasedGlobalCompilationDatabase::getCDBInDirLocked(PathRef Dir) const {
112+
// FIXME(ibiryukov): Invalidate cached compilation databases on changes
113+
auto Key = maybeCaseFoldPath(Dir);
114+
auto R = CompilationDatabases.try_emplace(Key);
115+
if (R.second) { // Cache miss, try to load CDB.
116+
CachedCDB &Entry = R.first->second;
117+
std::string Error;
118+
Entry.Path = std::string(Dir);
119+
Entry.CDB = tooling::CompilationDatabase::loadFromDirectory(Dir, Error);
120+
// Check for $src/build, the conventional CMake build root.
121+
// Probe existence first to avoid each plugin doing IO if it doesn't exist.
122+
if (!CompileCommandsDir && !Entry.CDB) {
123+
llvm::SmallString<256> BuildDir = Dir;
124+
llvm::sys::path::append(BuildDir, "build");
125+
if (llvm::sys::fs::is_directory(BuildDir)) {
126+
vlog("Found candidate build directory {0}", BuildDir);
127+
Entry.CDB =
128+
tooling::CompilationDatabase::loadFromDirectory(BuildDir, Error);
129+
}
130+
}
131+
if (Entry.CDB)
132+
log("Loaded compilation database from {0}", Dir);
230133
}
231-
232-
std::vector<DirectoryCache *> Ret;
233-
Ret.reserve(Dirs.size());
234-
235-
std::lock_guard<std::mutex> Lock(DirCachesMutex);
236-
for (unsigned I = 0; I < Dirs.size(); ++I)
237-
Ret.push_back(&DirCaches.try_emplace(FoldedDirs[I], Dirs[I]).first->second);
238-
return Ret;
134+
return R.first->second;
239135
}
240136

241137
llvm::Optional<DirectoryBasedGlobalCompilationDatabase::CDBLookupResult>
@@ -245,40 +141,39 @@ DirectoryBasedGlobalCompilationDatabase::lookupCDB(
245141
"path must be absolute");
246142

247143
bool ShouldBroadcast = false;
248-
DirectoryCache *DirCache = nullptr;
249-
std::shared_ptr<const tooling::CompilationDatabase> CDB = nullptr;
250-
if (OnlyDirCache) {
251-
DirCache = OnlyDirCache.get();
252-
ShouldBroadcast = Request.ShouldBroadcast;
253-
CDB = DirCache->get(ShouldBroadcast);
254-
} else {
255-
// Traverse the canonical version to prevent false positives. i.e.:
256-
// src/build/../a.cc can detect a CDB in /src/build if not canonicalized.
257-
std::string CanonicalPath = removeDots(Request.FileName);
258-
std::vector<llvm::StringRef> SearchDirs;
259-
actOnAllParentDirectories(CanonicalPath, [&](PathRef Path) {
260-
SearchDirs.push_back(Path);
261-
return false;
262-
});
263-
for (DirectoryCache *Candidate : getDirectoryCaches(SearchDirs)) {
264-
bool CandidateShouldBroadcast = Request.ShouldBroadcast;
265-
if ((CDB = Candidate->get(CandidateShouldBroadcast))) {
266-
DirCache = Candidate;
267-
ShouldBroadcast = CandidateShouldBroadcast;
268-
break;
269-
}
144+
CDBLookupResult Result;
145+
146+
{
147+
std::lock_guard<std::mutex> Lock(Mutex);
148+
CachedCDB *Entry = nullptr;
149+
if (CompileCommandsDir) {
150+
Entry = &getCDBInDirLocked(*CompileCommandsDir);
151+
} else {
152+
// Traverse the canonical version to prevent false positives. i.e.:
153+
// src/build/../a.cc can detect a CDB in /src/build if not canonicalized.
154+
// FIXME(sammccall): this loop is hot, use a union-find-like structure.
155+
actOnAllParentDirectories(removeDots(Request.FileName),
156+
[&](PathRef Path) {
157+
Entry = &getCDBInDirLocked(Path);
158+
return Entry->CDB != nullptr;
159+
});
270160
}
271-
}
272161

273-
if (!CDB)
274-
return llvm::None;
162+
if (!Entry || !Entry->CDB)
163+
return llvm::None;
275164

276-
CDBLookupResult Result;
277-
Result.CDB = std::move(CDB);
278-
Result.PI.SourceRoot = DirCache->path().str();
165+
// Mark CDB as broadcasted to make sure discovery is performed once.
166+
if (Request.ShouldBroadcast && !Entry->SentBroadcast) {
167+
Entry->SentBroadcast = true;
168+
ShouldBroadcast = true;
169+
}
170+
171+
Result.CDB = Entry->CDB.get();
172+
Result.PI.SourceRoot = Entry->Path;
173+
}
279174

280-
// FIXME: Maybe make the following part async, since this can block
281-
// retrieval of compile commands.
175+
// FIXME: Maybe make the following part async, since this can block retrieval
176+
// of compile commands.
282177
if (ShouldBroadcast)
283178
broadcastCDB(Result);
284179
return Result;
@@ -291,32 +186,29 @@ void DirectoryBasedGlobalCompilationDatabase::broadcastCDB(
291186
std::vector<std::string> AllFiles = Result.CDB->getAllFiles();
292187
// We assume CDB in CompileCommandsDir owns all of its entries, since we don't
293188
// perform any search in parent paths whenever it is set.
294-
if (OnlyDirCache) {
295-
assert(OnlyDirCache->path() == Result.PI.SourceRoot &&
189+
if (CompileCommandsDir) {
190+
assert(*CompileCommandsDir == Result.PI.SourceRoot &&
296191
"Trying to broadcast a CDB outside of CompileCommandsDir!");
297192
OnCommandChanged.broadcast(std::move(AllFiles));
298193
return;
299194
}
300195

301-
// Uniquify all parent directories of all files.
302196
llvm::StringMap<bool> DirectoryHasCDB;
303-
std::vector<llvm::StringRef> FileAncestors;
304-
for (llvm::StringRef File : AllFiles) {
305-
actOnAllParentDirectories(File, [&](PathRef Path) {
306-
auto It = DirectoryHasCDB.try_emplace(Path);
307-
// Already seen this path, and all of its parents.
308-
if (!It.second)
309-
return true;
310-
311-
FileAncestors.push_back(It.first->getKey());
312-
return pathEqual(Path, Result.PI.SourceRoot);
313-
});
314-
}
315-
// Work out which ones have CDBs in them.
316-
for (DirectoryCache *Dir : getDirectoryCaches(FileAncestors)) {
317-
bool ShouldBroadcast = false;
318-
if (Dir->get(ShouldBroadcast))
319-
DirectoryHasCDB.find(Dir->path())->setValue(true);
197+
{
198+
std::lock_guard<std::mutex> Lock(Mutex);
199+
// Uniquify all parent directories of all files.
200+
for (llvm::StringRef File : AllFiles) {
201+
actOnAllParentDirectories(File, [&](PathRef Path) {
202+
auto It = DirectoryHasCDB.try_emplace(Path);
203+
// Already seen this path, and all of its parents.
204+
if (!It.second)
205+
return true;
206+
207+
CachedCDB &Entry = getCDBInDirLocked(Path);
208+
It.first->second = Entry.CDB != nullptr;
209+
return pathEqual(Path, Result.PI.SourceRoot);
210+
});
211+
}
320212
}
321213

322214
std::vector<std::string> GovernedFiles;

‎clang-tools-extra/clangd/GlobalCompilationDatabase.h

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -81,33 +81,35 @@ class DirectoryBasedGlobalCompilationDatabase
8181
llvm::Optional<ProjectInfo> getProjectInfo(PathRef File) const override;
8282

8383
private:
84-
class DirectoryCache;
85-
// If there's an explicit CompileCommandsDir, cache of the CDB found there.
86-
mutable std::unique_ptr<DirectoryCache> OnlyDirCache;
87-
88-
// Keyed by possibly-case-folded directory path.
89-
// We can hand out pointers as they're stable and entries are never removed.
90-
// Empty if CompileCommandsDir is given (OnlyDirCache is used instead).
91-
mutable llvm::StringMap<DirectoryCache> DirCaches;
92-
// DirCaches access must be locked (unlike OnlyDirCache, which is threadsafe).
93-
mutable std::mutex DirCachesMutex;
94-
95-
std::vector<DirectoryCache *>
96-
getDirectoryCaches(llvm::ArrayRef<llvm::StringRef> Dirs) const;
84+
/// Caches compilation databases loaded from directories.
85+
struct CachedCDB {
86+
std::string Path; // Not case-folded.
87+
std::unique_ptr<clang::tooling::CompilationDatabase> CDB = nullptr;
88+
bool SentBroadcast = false;
89+
};
90+
CachedCDB &getCDBInDirLocked(PathRef File) const;
9791

9892
struct CDBLookupRequest {
9993
PathRef FileName;
10094
// Whether this lookup should trigger discovery of the CDB found.
10195
bool ShouldBroadcast = false;
10296
};
10397
struct CDBLookupResult {
104-
std::shared_ptr<const tooling::CompilationDatabase> CDB;
98+
tooling::CompilationDatabase *CDB = nullptr;
10599
ProjectInfo PI;
106100
};
107101
llvm::Optional<CDBLookupResult> lookupCDB(CDBLookupRequest Request) const;
108102

109103
// Performs broadcast on governed files.
110104
void broadcastCDB(CDBLookupResult Res) const;
105+
106+
mutable std::mutex Mutex;
107+
// Keyed by possibly-case-folded directory path.
108+
mutable llvm::StringMap<CachedCDB> CompilationDatabases;
109+
110+
/// Used for command argument pointing to folder where compile_commands.json
111+
/// is located.
112+
llvm::Optional<Path> CompileCommandsDir;
111113
};
112114

113115
/// Extracts system include search path from drivers matching QueryDriverGlobs

0 commit comments

Comments
 (0)