<div dir="ltr">Hi Douglas,<div><br></div><div>Thanks for letting me know! The test seemed to be too strict for windows. r343623 is an attempt to fix. If it still doesn't work, feel free to revert the commits, and I'll re-investigate on Thursday.</div><div><br></div><div>- Eric</div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Oct 2, 2018 at 9:25 PM <<a href="mailto:douglas.yung@sony.com">douglas.yung@sony.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Eric,<br>
<br>
One of the tests you added in this commit is causing a failure on the PS4 Windows bot:<br>
<br>
<a href="http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/20347/steps/test/logs/stdio" rel="noreferrer" target="_blank">http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/20347/steps/test/logs/stdio</a>:<br>
<br>
FAIL: Extra Tools Unit Tests :: clangd/./ClangdTests.exe/ClangdTests.PreambleVFSStatCache (87 of 344)<br>
******************** TEST 'Extra Tools Unit Tests :: clangd/./ClangdTests.exe/ClangdTests.PreambleVFSStatCache' FAILED ********************<br>
Note: Google Test filter = ClangdTests.PreambleVFSStatCache<br>
<br>
[==========] Running 1 test from 1 test case.<br>
<br>
[----------] Global test environment set-up.<br>
<br>
[----------] 1 test from ClangdTests<br>
<br>
[ RUN ] ClangdTests.PreambleVFSStatCache<br>
<br>
C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\tools\extra\unittests\clangd\ClangdTests.cpp(1026): error: Expected: CountStats["foo.h"]<br>
<br>
Which is: 2<br>
<br>
To be equal to: 1u<br>
<br>
Which is: 1<br>
<br>
[ FAILED ] ClangdTests.PreambleVFSStatCache (441 ms)<br>
<br>
[----------] 1 test from ClangdTests (441 ms total)<br>
<br>
<br>
<br>
[----------] Global test environment tear-down<br>
<br>
[==========] 1 test from 1 test case ran. (441 ms total)<br>
<br>
[ PASSED ] 0 tests.<br>
<br>
[ FAILED ] 1 test, listed below:<br>
<br>
[ FAILED ] ClangdTests.PreambleVFSStatCache<br>
<br>
<br>
<br>
1 FAILED TEST<br>
<br>
Updating file C:\clangd-test\foo.cpp with command [C:\clangd-test] clang -ffreestanding C:\clangd-test\foo.cpp -resource-dir=C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.obj\tools\clang\tools\extra\unittests\clangd\..\lib\clang\8.0.0<br>
<br>
Preamble for file C:\clangd-test\foo.cpp cannot be reused. Attempting to rebuild it.<br>
<br>
Built preamble of size 185492 for file C:\clangd-test\foo.cpp<br>
<br>
Code complete: sema context Statement, query scopes [] (AnyScope=false)<br>
<br>
Code complete: 1 results from Sema, 0 from Index, 0 matched, 1 returned.<br>
<br>
********************<br>
<br>
Can you please take a look?<br>
<br>
Douglas Yung<br>
<br>
> -----Original Message-----<br>
> From: cfe-commits [mailto:<a href="mailto:cfe-commits-bounces@lists.llvm.org" target="_blank">cfe-commits-bounces@lists.llvm.org</a>] On Behalf<br>
> Of Eric Liu via cfe-commits<br>
> Sent: Tuesday, October 02, 2018 3:44<br>
> To: <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
> Subject: [clang-tools-extra] r343576 - [clangd] Cache FS stat() calls<br>
> when building preamble.<br>
> <br>
> Author: ioeric<br>
> Date: Tue Oct 2 03:43:55 2018<br>
> New Revision: 343576<br>
> <br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=343576&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=343576&view=rev</a><br>
> Log:<br>
> [clangd] Cache FS stat() calls when building preamble.<br>
> <br>
> Summary:<br>
> The file stats can be reused when preamble is reused (e.g. code<br>
> completion). It's safe to assume that cached status is not outdated as<br>
> we<br>
> assume preamble files to remain unchanged.<br>
> <br>
> On real file system, this made code completion ~20% faster on a<br>
> measured file<br>
> (with big preamble). The preamble build time doesn't change much.<br>
> <br>
> Reviewers: sammccall, ilya-biryukov<br>
> <br>
> Reviewed By: sammccall<br>
> <br>
> Subscribers: mgorny, MaskRay, jkorous, arphaman, kadircet, cfe-commits<br>
> <br>
> Differential Revision: <a href="https://reviews.llvm.org/D52419" rel="noreferrer" target="_blank">https://reviews.llvm.org/D52419</a><br>
> <br>
> Added:<br>
> clang-tools-extra/trunk/clangd/FS.cpp<br>
> clang-tools-extra/trunk/clangd/FS.h<br>
> clang-tools-extra/trunk/unittests/clangd/FSTests.cpp<br>
> Modified:<br>
> clang-tools-extra/trunk/clangd/CMakeLists.txt<br>
> clang-tools-extra/trunk/clangd/ClangdServer.cpp<br>
> clang-tools-extra/trunk/clangd/ClangdUnit.cpp<br>
> clang-tools-extra/trunk/clangd/ClangdUnit.h<br>
> clang-tools-extra/trunk/clangd/CodeComplete.cpp<br>
> clang-tools-extra/trunk/clangd/CodeComplete.h<br>
> clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt<br>
> clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp<br>
> clang-tools-extra/trunk/unittests/clangd/TestFS.cpp<br>
> <br>
> Modified: clang-tools-extra/trunk/clangd/CMakeLists.txt<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-</a><br>
> extra/trunk/clangd/CMakeLists.txt?rev=343576&r1=343575&r2=343576&view=d<br>
> iff<br>
> =======================================================================<br>
> =======<br>
> --- clang-tools-extra/trunk/clangd/CMakeLists.txt (original)<br>
> +++ clang-tools-extra/trunk/clangd/CMakeLists.txt Tue Oct 2 03:43:55<br>
> 2018<br>
> @@ -21,6 +21,7 @@ add_clang_library(clangDaemon<br>
> DraftStore.cpp<br>
> FindSymbols.cpp<br>
> FileDistance.cpp<br>
> + FS.cpp<br>
> FuzzyMatch.cpp<br>
> GlobalCompilationDatabase.cpp<br>
> Headers.cpp<br>
> <br>
> Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-</a><br>
> extra/trunk/clangd/ClangdServer.cpp?rev=343576&r1=343575&r2=343576&view<br>
> =diff<br>
> =======================================================================<br>
> =======<br>
> --- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)<br>
> +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Tue Oct 2 03:43:55<br>
> 2018<br>
> @@ -181,8 +181,6 @@ void ClangdServer::codeComplete(PathRef<br>
> if (isCancelled())<br>
> return CB(llvm::make_error<CancelledError>());<br>
> <br>
> - auto PreambleData = IP->Preamble;<br>
> -<br>
> llvm::Optional<SpeculativeFuzzyFind> SpecFuzzyFind;<br>
> if (CodeCompleteOpts.Index &&<br>
> CodeCompleteOpts.SpeculativeIndexRequest) {<br>
> SpecFuzzyFind.emplace();<br>
> @@ -195,10 +193,8 @@ void ClangdServer::codeComplete(PathRef<br>
> // FIXME(ibiryukov): even if Preamble is non-null, we may want to<br>
> check<br>
> // both the old and the new version in case only one of them<br>
> matches.<br>
> CodeCompleteResult Result = clangd::codeComplete(<br>
> - File, IP->Command, PreambleData ? &PreambleData->Preamble :<br>
> nullptr,<br>
> - PreambleData ? PreambleData->Includes : IncludeStructure(),<br>
> - IP->Contents, Pos, FS, PCHs, CodeCompleteOpts,<br>
> - SpecFuzzyFind ? SpecFuzzyFind.getPointer() : nullptr);<br>
> + File, IP->Command, IP->Preamble, IP->Contents, Pos, FS, PCHs,<br>
> + CodeCompleteOpts, SpecFuzzyFind ? SpecFuzzyFind.getPointer() :<br>
> nullptr);<br>
> {<br>
> clang::clangd::trace::Span Tracer("Completion results<br>
> callback");<br>
> CB(std::move(Result));<br>
> @@ -231,9 +227,8 @@ void ClangdServer::signatureHelp(PathRef<br>
> return CB(IP.takeError());<br>
> <br>
> auto PreambleData = IP->Preamble;<br>
> - CB(clangd::signatureHelp(File, IP->Command,<br>
> - PreambleData ? &PreambleData->Preamble :<br>
> nullptr,<br>
> - IP->Contents, Pos, FS, PCHs, Index));<br>
> + CB(clangd::signatureHelp(File, IP->Command, PreambleData, IP-<br>
> >Contents, Pos,<br>
> + FS, PCHs, Index));<br>
> };<br>
> <br>
> // Unlike code completion, we wait for an up-to-date preamble here.<br>
> <br>
> Modified: clang-tools-extra/trunk/clangd/ClangdUnit.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-</a><br>
> extra/trunk/clangd/ClangdUnit.cpp?rev=343576&r1=343575&r2=343576&view=d<br>
> iff<br>
> =======================================================================<br>
> =======<br>
> --- clang-tools-extra/trunk/clangd/ClangdUnit.cpp (original)<br>
> +++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp Tue Oct 2 03:43:55<br>
> 2018<br>
> @@ -246,9 +246,10 @@ const IncludeStructure &ParsedAST::getIn<br>
> }<br>
> <br>
> PreambleData::PreambleData(PrecompiledPreamble Preamble,<br>
> - std::vector<Diag> Diags, IncludeStructure<br>
> Includes)<br>
> + std::vector<Diag> Diags, IncludeStructure<br>
> Includes,<br>
> + std::unique_ptr<PreambleFileStatusCache><br>
> StatCache)<br>
> : Preamble(std::move(Preamble)), Diags(std::move(Diags)),<br>
> - Includes(std::move(Includes)) {}<br>
> + Includes(std::move(Includes)), StatCache(std::move(StatCache))<br>
> {}<br>
> <br>
> ParsedAST::ParsedAST(std::shared_ptr<const PreambleData> Preamble,<br>
> std::unique_ptr<CompilerInstance> Clang,<br>
> @@ -334,9 +335,12 @@ std::shared_ptr<const PreambleData> clan<br>
> // We proceed anyway, our lit-tests rely on results for non-<br>
> existing working<br>
> // dirs.<br>
> }<br>
> +<br>
> + auto StatCache = llvm::make_unique<PreambleFileStatusCache>();<br>
> auto BuiltPreamble = PrecompiledPreamble::Build(<br>
> - CI, ContentsBuffer.get(), Bounds, *PreambleDiagsEngine,<br>
> Inputs.FS, PCHs,<br>
> - StoreInMemory, SerializedDeclsCollector);<br>
> + CI, ContentsBuffer.get(), Bounds, *PreambleDiagsEngine,<br>
> + StatCache->getProducingFS(Inputs.FS), PCHs, StoreInMemory,<br>
> + SerializedDeclsCollector);<br>
> <br>
> // When building the AST for the main file, we do want the function<br>
> // bodies.<br>
> @@ -347,7 +351,7 @@ std::shared_ptr<const PreambleData> clan<br>
> FileName);<br>
> return std::make_shared<PreambleData>(<br>
> std::move(*BuiltPreamble), PreambleDiagnostics.take(),<br>
> - SerializedDeclsCollector.takeIncludes());<br>
> + SerializedDeclsCollector.takeIncludes(),<br>
> std::move(StatCache));<br>
> } else {<br>
> elog("Could not build a preamble for file {0}", FileName);<br>
> return nullptr;<br>
> @@ -361,15 +365,19 @@ llvm::Optional<ParsedAST> clangd::buildA<br>
> trace::Span Tracer("BuildAST");<br>
> SPAN_ATTACH(Tracer, "File", FileName);<br>
> <br>
> - if (Inputs.FS-<br>
> >setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {<br>
> + auto VFS = Inputs.FS;<br>
> + if (Preamble && Preamble->StatCache)<br>
> + VFS = Preamble->StatCache->getConsumingFS(std::move(VFS));<br>
> + if (VFS-<br>
> >setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {<br>
> log("Couldn't set working directory when building the preamble.");<br>
> // We proceed anyway, our lit-tests rely on results for non-<br>
> existing working<br>
> // dirs.<br>
> }<br>
> <br>
> - return ParsedAST::build(<br>
> - llvm::make_unique<CompilerInvocation>(*Invocation), Preamble,<br>
> - llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents), PCHs,<br>
> Inputs.FS);<br>
> + return<br>
> ParsedAST::build(llvm::make_unique<CompilerInvocation>(*Invocation),<br>
> + Preamble,<br>
> +<br>
> llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents),<br>
> + PCHs, std::move(VFS));<br>
> }<br>
> <br>
> SourceLocation clangd::getBeginningOfIdentifier(ParsedAST &Unit,<br>
> <br>
> Modified: clang-tools-extra/trunk/clangd/ClangdUnit.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-</a><br>
> extra/trunk/clangd/ClangdUnit.h?rev=343576&r1=343575&r2=343576&view=dif<br>
> f<br>
> =======================================================================<br>
> =======<br>
> --- clang-tools-extra/trunk/clangd/ClangdUnit.h (original)<br>
> +++ clang-tools-extra/trunk/clangd/ClangdUnit.h Tue Oct 2 03:43:55<br>
> 2018<br>
> @@ -11,6 +11,7 @@<br>
> #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDUNIT_H<br>
> <br>
> #include "Diagnostics.h"<br>
> +#include "FS.h"<br>
> #include "Function.h"<br>
> #include "Headers.h"<br>
> #include "Path.h"<br>
> @@ -45,7 +46,8 @@ namespace clangd {<br>
> // Stores Preamble and associated data.<br>
> struct PreambleData {<br>
> PreambleData(PrecompiledPreamble Preamble, std::vector<Diag> Diags,<br>
> - IncludeStructure Includes);<br>
> + IncludeStructure Includes,<br>
> + std::unique_ptr<PreambleFileStatusCache> StatCache);<br>
> <br>
> tooling::CompileCommand CompileCommand;<br>
> PrecompiledPreamble Preamble;<br>
> @@ -53,6 +55,9 @@ struct PreambleData {<br>
> // Processes like code completions and go-to-definitions will need<br>
> #include<br>
> // information, and their compile action skips preamble range.<br>
> IncludeStructure Includes;<br>
> + // Cache of FS operations performed when building the preamble.<br>
> + // When reusing a preamble, this cache can be consumed to save IO.<br>
> + std::unique_ptr<PreambleFileStatusCache> StatCache;<br>
> };<br>
> <br>
> /// Information required to run clang, e.g. to parse AST or do code<br>
> completion.<br>
> <br>
> Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-</a><br>
> extra/trunk/clangd/CodeComplete.cpp?rev=343576&r1=343575&r2=343576&view<br>
> =diff<br>
> =======================================================================<br>
> =======<br>
> --- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)<br>
> +++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Tue Oct 2 03:43:55<br>
> 2018<br>
> @@ -20,6 +20,7 @@<br>
> <br>
> #include "CodeComplete.h"<br>
> #include "AST.h"<br>
> +#include "ClangdUnit.h"<br>
> #include "CodeCompletionStrings.h"<br>
> #include "Compiler.h"<br>
> #include "Diagnostics.h"<br>
> @@ -986,7 +987,7 @@ private:<br>
> struct SemaCompleteInput {<br>
> PathRef FileName;<br>
> const tooling::CompileCommand &Command;<br>
> - PrecompiledPreamble const *Preamble;<br>
> + const PreambleData *Preamble;<br>
> StringRef Contents;<br>
> Position Pos;<br>
> IntrusiveRefCntPtr<vfs::FileSystem> VFS;<br>
> @@ -1010,12 +1011,15 @@ bool semaCodeComplete(std::unique_ptr<Co<br>
> // working dirs.<br>
> }<br>
> <br>
> + IntrusiveRefCntPtr<vfs::FileSystem> VFS = Input.VFS;<br>
> + if (Input.Preamble && Input.Preamble->StatCache)<br>
> + VFS = Input.Preamble->StatCache->getConsumingFS(std::move(VFS));<br>
> IgnoreDiagnostics DummyDiagsConsumer;<br>
> auto CI = createInvocationFromCommandLine(<br>
> ArgStrs,<br>
> CompilerInstance::createDiagnostics(new DiagnosticOptions,<br>
> &DummyDiagsConsumer, false),<br>
> - Input.VFS);<br>
> + VFS);<br>
> if (!CI) {<br>
> elog("Couldn't create CompilerInvocation");<br>
> return false;<br>
> @@ -1054,8 +1058,10 @@ bool semaCodeComplete(std::unique_ptr<Co<br>
> // NOTE: we must call BeginSourceFile after prepareCompilerInstance.<br>
> Otherwise<br>
> // the remapped buffers do not get freed.<br>
> auto Clang = prepareCompilerInstance(<br>
> - std::move(CI), CompletingInPreamble ? nullptr : Input.Preamble,<br>
> - std::move(ContentsBuffer), std::move(Input.PCHs),<br>
> std::move(Input.VFS),<br>
> + std::move(CI),<br>
> + (Input.Preamble && !CompletingInPreamble) ? &Input.Preamble-<br>
> >Preamble<br>
> + : nullptr,<br>
> + std::move(ContentsBuffer), std::move(Input.PCHs),<br>
> std::move(VFS),<br>
> DummyDiagsConsumer);<br>
> Clang->getPreprocessorOpts().SingleFileParseMode =<br>
> CompletingInPreamble;<br>
> Clang->setCodeCompletionConsumer(Consumer.release());<br>
> @@ -1565,19 +1571,20 @@ speculateCompletionFilter(llvm::StringRe<br>
> <br>
> CodeCompleteResult<br>
> codeComplete(PathRef FileName, const tooling::CompileCommand &Command,<br>
> - PrecompiledPreamble const *Preamble,<br>
> - const IncludeStructure &PreambleInclusions, StringRef<br>
> Contents,<br>
> - Position Pos, IntrusiveRefCntPtr<vfs::FileSystem> VFS,<br>
> + const PreambleData *Preamble, StringRef Contents,<br>
> Position Pos,<br>
> + IntrusiveRefCntPtr<vfs::FileSystem> VFS,<br>
> std::shared_ptr<PCHContainerOperations> PCHs,<br>
> CodeCompleteOptions Opts, SpeculativeFuzzyFind<br>
> *SpecFuzzyFind) {<br>
> - return CodeCompleteFlow(FileName, PreambleInclusions, SpecFuzzyFind,<br>
> Opts)<br>
> + return CodeCompleteFlow(FileName,<br>
> + Preamble ? Preamble->Includes :<br>
> IncludeStructure(),<br>
> + SpecFuzzyFind, Opts)<br>
> .run({FileName, Command, Preamble, Contents, Pos, VFS, PCHs});<br>
> }<br>
> <br>
> SignatureHelp signatureHelp(PathRef FileName,<br>
> const tooling::CompileCommand &Command,<br>
> - PrecompiledPreamble const *Preamble,<br>
> - StringRef Contents, Position Pos,<br>
> + const PreambleData *Preamble, StringRef<br>
> Contents,<br>
> + Position Pos,<br>
> IntrusiveRefCntPtr<vfs::FileSystem> VFS,<br>
> std::shared_ptr<PCHContainerOperations><br>
> PCHs,<br>
> const SymbolIndex *Index) {<br>
> <br>
> Modified: clang-tools-extra/trunk/clangd/CodeComplete.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-</a><br>
> extra/trunk/clangd/CodeComplete.h?rev=343576&r1=343575&r2=343576&view=d<br>
> iff<br>
> =======================================================================<br>
> =======<br>
> --- clang-tools-extra/trunk/clangd/CodeComplete.h (original)<br>
> +++ clang-tools-extra/trunk/clangd/CodeComplete.h Tue Oct 2 03:43:55<br>
> 2018<br>
> @@ -16,6 +16,7 @@<br>
> #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CODECOMPLETE_H<br>
> #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CODECOMPLETE_H<br>
> <br>
> +#include "ClangdUnit.h"<br>
> #include "Headers.h"<br>
> #include "Logger.h"<br>
> #include "Path.h"<br>
> @@ -221,8 +222,7 @@ struct SpeculativeFuzzyFind {<br>
> /// destroyed when the async request finishes.<br>
> CodeCompleteResult codeComplete(PathRef FileName,<br>
> const tooling::CompileCommand<br>
> &Command,<br>
> - PrecompiledPreamble const *Preamble,<br>
> - const IncludeStructure<br>
> &PreambleInclusions,<br>
> + const PreambleData *Preamble,<br>
> StringRef Contents, Position Pos,<br>
> IntrusiveRefCntPtr<vfs::FileSystem><br>
> VFS,<br>
> <br>
> std::shared_ptr<PCHContainerOperations> PCHs,<br>
> @@ -232,8 +232,8 @@ CodeCompleteResult codeComplete(PathRef<br>
> /// Get signature help at a specified \p Pos in \p FileName.<br>
> SignatureHelp signatureHelp(PathRef FileName,<br>
> const tooling::CompileCommand &Command,<br>
> - PrecompiledPreamble const *Preamble,<br>
> - StringRef Contents, Position Pos,<br>
> + const PreambleData *Preamble, StringRef<br>
> Contents,<br>
> + Position Pos,<br>
> IntrusiveRefCntPtr<vfs::FileSystem> VFS,<br>
> std::shared_ptr<PCHContainerOperations><br>
> PCHs,<br>
> const SymbolIndex *Index);<br>
> <br>
> Added: clang-tools-extra/trunk/clangd/FS.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-</a><br>
> extra/trunk/clangd/FS.cpp?rev=343576&view=auto<br>
> =======================================================================<br>
> =======<br>
> --- clang-tools-extra/trunk/clangd/FS.cpp (added)<br>
> +++ clang-tools-extra/trunk/clangd/FS.cpp Tue Oct 2 03:43:55 2018<br>
> @@ -0,0 +1,92 @@<br>
> +//===--- FS.cpp - File system related utils ----------------------*-<br>
> C++-*-===//<br>
> +//<br>
> +// The LLVM Compiler Infrastructure<br>
> +//<br>
> +// This file is distributed under the University of Illinois Open<br>
> Source<br>
> +// License. See LICENSE.TXT for details.<br>
> +//<br>
> +//===-----------------------------------------------------------------<br>
> -----===//<br>
> +<br>
> +#include "FS.h"<br>
> +#include "clang/Basic/VirtualFileSystem.h"<br>
> +#include "llvm/ADT/None.h"<br>
> +<br>
> +namespace clang {<br>
> +namespace clangd {<br>
> +<br>
> +void PreambleFileStatusCache::update(const vfs::FileSystem &FS,<br>
> vfs::Status S) {<br>
> + SmallString<32> PathStore(S.getName());<br>
> + if (auto Err = FS.makeAbsolute(PathStore))<br>
> + return;<br>
> + // Stores the latest status in cache as it can change in a preamble<br>
> build.<br>
> + StatCache.insert({PathStore, std::move(S)});<br>
> +}<br>
> +<br>
> +llvm::Optional<vfs::Status><br>
> +PreambleFileStatusCache::lookup(llvm::StringRef File) const {<br>
> + auto I = StatCache.find(File);<br>
> + if (I != StatCache.end())<br>
> + return I->getValue();<br>
> + return llvm::None;<br>
> +}<br>
> +<br>
> +IntrusiveRefCntPtr<vfs::FileSystem><br>
> PreambleFileStatusCache::getProducingFS(<br>
> + IntrusiveRefCntPtr<vfs::FileSystem> FS) {<br>
> + // This invalidates old status in cache if files are re-`open()`ed<br>
> or<br>
> + // re-`stat()`ed in case file status has changed during preamble<br>
> build.<br>
> + class CollectFS : public vfs::ProxyFileSystem {<br>
> + public:<br>
> + CollectFS(IntrusiveRefCntPtr<vfs::FileSystem> FS,<br>
> + PreambleFileStatusCache &StatCache)<br>
> + : ProxyFileSystem(std::move(FS)), StatCache(StatCache) {}<br>
> +<br>
> + llvm::ErrorOr<std::unique_ptr<vfs::File>><br>
> + openFileForRead(const Twine &Path) override {<br>
> + auto File = getUnderlyingFS().openFileForRead(Path);<br>
> + if (!File || !*File)<br>
> + return File;<br>
> + // Eagerly stat opened file, as the followup `status` call on<br>
> the file<br>
> + // doesn't necessarily go through this FS. This puts some extra<br>
> work on<br>
> + // preamble build, but it should be worth it as preamble can be<br>
> reused<br>
> + // many times (e.g. code completion) and the repeated status<br>
> call is<br>
> + // likely to be cached in the underlying file system anyway.<br>
> + if (auto S = File->get()->status())<br>
> + StatCache.update(getUnderlyingFS(), std::move(*S));<br>
> + return File;<br>
> + }<br>
> +<br>
> + llvm::ErrorOr<vfs::Status> status(const Twine &Path) override {<br>
> + auto S = getUnderlyingFS().status(Path);<br>
> + if (S)<br>
> + StatCache.update(getUnderlyingFS(), *S);<br>
> + return S;<br>
> + }<br>
> +<br>
> + private:<br>
> + PreambleFileStatusCache &StatCache;<br>
> + };<br>
> + return IntrusiveRefCntPtr<CollectFS>(new CollectFS(std::move(FS),<br>
> *this));<br>
> +}<br>
> +<br>
> +IntrusiveRefCntPtr<vfs::FileSystem><br>
> PreambleFileStatusCache::getConsumingFS(<br>
> + IntrusiveRefCntPtr<vfs::FileSystem> FS) const {<br>
> + class CacheVFS : public vfs::ProxyFileSystem {<br>
> + public:<br>
> + CacheVFS(IntrusiveRefCntPtr<vfs::FileSystem> FS,<br>
> + const PreambleFileStatusCache &StatCache)<br>
> + : ProxyFileSystem(std::move(FS)), StatCache(StatCache) {}<br>
> +<br>
> + llvm::ErrorOr<vfs::Status> status(const Twine &Path) override {<br>
> + if (auto S = StatCache.lookup(Path.str()))<br>
> + return *S;<br>
> + return getUnderlyingFS().status(Path);<br>
> + }<br>
> +<br>
> + private:<br>
> + const PreambleFileStatusCache &StatCache;<br>
> + };<br>
> + return IntrusiveRefCntPtr<CacheVFS>(new CacheVFS(std::move(FS),<br>
> *this));<br>
> +}<br>
> +<br>
> +} // namespace clangd<br>
> +} // namespace clang<br>
> <br>
> Added: clang-tools-extra/trunk/clangd/FS.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-</a><br>
> extra/trunk/clangd/FS.h?rev=343576&view=auto<br>
> =======================================================================<br>
> =======<br>
> --- clang-tools-extra/trunk/clangd/FS.h (added)<br>
> +++ clang-tools-extra/trunk/clangd/FS.h Tue Oct 2 03:43:55 2018<br>
> @@ -0,0 +1,65 @@<br>
> +//===--- FS.h - File system related utils ------------------------*-<br>
> C++-*-===//<br>
> +//<br>
> +// The LLVM Compiler Infrastructure<br>
> +//<br>
> +// This file is distributed under the University of Illinois Open<br>
> Source<br>
> +// License. See LICENSE.TXT for details.<br>
> +//<br>
> +//===-----------------------------------------------------------------<br>
> -----===//<br>
> +<br>
> +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FS_H<br>
> +#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FS_H<br>
> +<br>
> +#include "clang/Basic/VirtualFileSystem.h"<br>
> +#include "llvm/ADT/Optional.h"<br>
> +<br>
> +namespace clang {<br>
> +namespace clangd {<br>
> +<br>
> +/// Records status information for files open()ed or stat()ed during<br>
> preamble<br>
> +/// build, so we can avoid stat()s on the underlying FS when reusing<br>
> the<br>
> +/// preamble. For example, code completion can re-stat files when<br>
> getting FileID<br>
> +/// for source locations stored in preamble (e.g. checking whether a<br>
> location is<br>
> +/// in the main file).<br>
> +///<br>
> +/// The cache is keyed by absolute path of file name in cached status,<br>
> as this<br>
> +/// is what preamble stores.<br>
> +///<br>
> +/// The cache is not thread-safe when updates happen, so the use<br>
> pattern should<br>
> +/// be:<br>
> +/// - One FS writes to the cache from one thread (or several but<br>
> strictly<br>
> +/// sequenced), e.g. when building preamble.<br>
> +/// - Sequence point (no writes after this point, no reads before).<br>
> +/// - Several FSs can read from the cache, e.g. code completions.<br>
> +///<br>
> +/// Note that the cache is only valid when reusing preamble.<br>
> +class PreambleFileStatusCache {<br>
> +public:<br>
> + void update(const vfs::FileSystem &FS, vfs::Status S);<br>
> + /// \p Path is a path stored in preamble.<br>
> + llvm::Optional<vfs::Status> lookup(llvm::StringRef Path) const;<br>
> +<br>
> + /// Returns a VFS that collects file status.<br>
> + /// Only cache stats for files that exist because<br>
> + /// 1) we only care about existing files when reusing preamble,<br>
> unlike<br>
> + /// building preamble.<br>
> + /// 2) we use the file name in the Status as the cache key.<br>
> + ///<br>
> + /// Note that the returned VFS should not outlive the cache.<br>
> + IntrusiveRefCntPtr<vfs::FileSystem><br>
> + getProducingFS(IntrusiveRefCntPtr<vfs::FileSystem> FS);<br>
> +<br>
> + /// Returns a VFS that uses the cache collected.<br>
> + ///<br>
> + /// Note that the returned VFS should not outlive the cache.<br>
> + IntrusiveRefCntPtr<vfs::FileSystem><br>
> + getConsumingFS(IntrusiveRefCntPtr<vfs::FileSystem> FS) const;<br>
> +<br>
> +private:<br>
> + llvm::StringMap<vfs::Status> StatCache;<br>
> +};<br>
> +<br>
> +} // namespace clangd<br>
> +} // namespace clang<br>
> +<br>
> +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_FS_H<br>
> <br>
> Modified: clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-</a><br>
> extra/trunk/unittests/clangd/CMakeLists.txt?rev=343576&r1=343575&r2=343<br>
> 576&view=diff<br>
> =======================================================================<br>
> =======<br>
> --- clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt (original)<br>
> +++ clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt Tue Oct 2<br>
> 03:43:55 2018<br>
> @@ -21,6 +21,7 @@ add_extra_unittest(ClangdTests<br>
> FileDistanceTests.cpp<br>
> FileIndexTests.cpp<br>
> FindSymbolsTests.cpp<br>
> + FSTests.cpp<br>
> FuzzyMatchTests.cpp<br>
> GlobalCompilationDatabaseTests.cpp<br>
> HeadersTests.cpp<br>
> <br>
> Modified: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-</a><br>
> extra/trunk/unittests/clangd/ClangdTests.cpp?rev=343576&r1=343575&r2=34<br>
> 3576&view=diff<br>
> =======================================================================<br>
> =======<br>
> --- clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp (original)<br>
> +++ clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp Tue Oct 2<br>
> 03:43:55 2018<br>
> @@ -963,6 +963,71 @@ TEST_F(ClangdVFSTest, ChangedHeaderFromI<br>
> Field(&CodeCompletion::Name,<br>
> "baz")));<br>
> }<br>
> <br>
> +// Check that running code completion doesn't stat() a bunch of files<br>
> from the<br>
> +// preamble again. (They should be using the preamble's stat-cache)<br>
> +TEST(ClangdTests, PreambleVFSStatCache) {<br>
> + class ListenStatsFSProvider : public FileSystemProvider {<br>
> + public:<br>
> + ListenStatsFSProvider(llvm::StringMap<unsigned> &CountStats)<br>
> + : CountStats(CountStats) {}<br>
> +<br>
> + IntrusiveRefCntPtr<vfs::FileSystem> getFileSystem() override {<br>
> + class ListenStatVFS : public vfs::ProxyFileSystem {<br>
> + public:<br>
> + ListenStatVFS(IntrusiveRefCntPtr<vfs::FileSystem> FS,<br>
> + llvm::StringMap<unsigned> &CountStats)<br>
> + : ProxyFileSystem(std::move(FS)), CountStats(CountStats)<br>
> {}<br>
> +<br>
> + llvm::ErrorOr<std::unique_ptr<vfs::File>><br>
> + openFileForRead(const Twine &Path) override {<br>
> + ++CountStats[llvm::sys::path::filename(Path.str())];<br>
> + return ProxyFileSystem::openFileForRead(Path);<br>
> + }<br>
> + llvm::ErrorOr<vfs::Status> status(const Twine &Path) override<br>
> {<br>
> + ++CountStats[llvm::sys::path::filename(Path.str())];<br>
> + return ProxyFileSystem::status(Path);<br>
> + }<br>
> +<br>
> + private:<br>
> + llvm::StringMap<unsigned> &CountStats;<br>
> + };<br>
> +<br>
> + return IntrusiveRefCntPtr<ListenStatVFS>(<br>
> + new ListenStatVFS(buildTestFS(Files), CountStats));<br>
> + }<br>
> +<br>
> + // If relative paths are used, they are resolved with testPath().<br>
> + llvm::StringMap<std::string> Files;<br>
> + llvm::StringMap<unsigned> &CountStats;<br>
> + };<br>
> +<br>
> + llvm::StringMap<unsigned> CountStats;<br>
> + ListenStatsFSProvider FS(CountStats);<br>
> + ErrorCheckingDiagConsumer DiagConsumer;<br>
> + MockCompilationDatabase CDB;<br>
> + ClangdServer Server(CDB, FS, DiagConsumer,<br>
> ClangdServer::optsForTest());<br>
> +<br>
> + auto SourcePath = testPath("foo.cpp");<br>
> + auto HeaderPath = testPath("foo.h");<br>
> + FS.Files[HeaderPath] = "struct TestSym {};";<br>
> + Annotations Code(R"cpp(<br>
> + #include "foo.h"<br>
> +<br>
> + int main() {<br>
> + TestSy^<br>
> + })cpp");<br>
> +<br>
> + runAddDocument(Server, SourcePath, Code.code());<br>
> +<br>
> + EXPECT_EQ(CountStats["foo.h"], 1u);<br>
> + auto Completions = cantFail(runCodeComplete(Server, SourcePath,<br>
> Code.point(),<br>
> +<br>
> clangd::CodeCompleteOptions()))<br>
> + .Completions;<br>
> + EXPECT_EQ(CountStats["foo.h"], 1u);<br>
> + EXPECT_THAT(Completions,<br>
> + ElementsAre(Field(&CodeCompletion::Name, "TestSym")));<br>
> +}<br>
> +<br>
> } // namespace<br>
> } // namespace clangd<br>
> } // namespace clang<br>
> <br>
> Added: clang-tools-extra/trunk/unittests/clangd/FSTests.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-</a><br>
> extra/trunk/unittests/clangd/FSTests.cpp?rev=343576&view=auto<br>
> =======================================================================<br>
> =======<br>
> --- clang-tools-extra/trunk/unittests/clangd/FSTests.cpp (added)<br>
> +++ clang-tools-extra/trunk/unittests/clangd/FSTests.cpp Tue Oct 2<br>
> 03:43:55 2018<br>
> @@ -0,0 +1,46 @@<br>
> +//===-- FSTests.cpp - File system related tests -----------------*-<br>
> C++ -*-===//<br>
> +//<br>
> +// The LLVM Compiler Infrastructure<br>
> +//<br>
> +// This file is distributed under the University of Illinois Open<br>
> Source<br>
> +// License. See LICENSE.TXT for details.<br>
> +//<br>
> +//===-----------------------------------------------------------------<br>
> -----===//<br>
> +<br>
> +#include "FS.h"<br>
> +#include "TestFS.h"<br>
> +#include "gmock/gmock.h"<br>
> +#include "gtest/gtest.h"<br>
> +<br>
> +namespace clang {<br>
> +namespace clangd {<br>
> +namespace {<br>
> +<br>
> +TEST(FSTests, PreambleStatusCache) {<br>
> + llvm::StringMap<std::string> Files;<br>
> + Files["x"] = "";<br>
> + Files["y"] = "";<br>
> + auto FS = buildTestFS(Files);<br>
> + FS->setCurrentWorkingDirectory(testRoot());<br>
> +<br>
> + PreambleFileStatusCache StatCache;<br>
> + auto ProduceFS = StatCache.getProducingFS(FS);<br>
> + EXPECT_TRUE(ProduceFS->openFileForRead("x"));<br>
> + EXPECT_TRUE(ProduceFS->status("y"));<br>
> +<br>
> + EXPECT_TRUE(StatCache.lookup(testPath("x")).hasValue());<br>
> + EXPECT_TRUE(StatCache.lookup(testPath("y")).hasValue());<br>
> +<br>
> + vfs::Status S("fake", llvm::sys::fs::UniqueID(0, 0),<br>
> + std::chrono::system_clock::now(), 0, 0, 1024,<br>
> + llvm::sys::fs::file_type::regular_file,<br>
> llvm::sys::fs::all_all);<br>
> + StatCache.update(*FS, S);<br>
> + auto ConsumeFS = StatCache.getConsumingFS(FS);<br>
> + auto Cached = ConsumeFS->status(testPath("fake"));<br>
> + EXPECT_TRUE(Cached);<br>
> + EXPECT_EQ(Cached->getName(), S.getName());<br>
> +}<br>
> +<br>
> +} // namespace<br>
> +} // namespace clangd<br>
> +} // namespace clang<br>
> <br>
> Modified: clang-tools-extra/trunk/unittests/clangd/TestFS.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-</a><br>
> extra/trunk/unittests/clangd/TestFS.cpp?rev=343576&r1=343575&r2=343576&<br>
> view=diff<br>
> =======================================================================<br>
> =======<br>
> --- clang-tools-extra/trunk/unittests/clangd/TestFS.cpp (original)<br>
> +++ clang-tools-extra/trunk/unittests/clangd/TestFS.cpp Tue Oct 2<br>
> 03:43:55 2018<br>
> @@ -23,6 +23,7 @@ buildTestFS(llvm::StringMap<std::string><br>
> llvm::StringMap<time_t> const &Timestamps) {<br>
> IntrusiveRefCntPtr<vfs::InMemoryFileSystem> MemFS(<br>
> new vfs::InMemoryFileSystem);<br>
> + MemFS->setCurrentWorkingDirectory(testRoot());<br>
> for (auto &FileAndContents : Files) {<br>
> StringRef File = FileAndContents.first();<br>
> MemFS->addFile(<br>
> <br>
> <br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>