[PATCH] D51638: [clangd] Load static index asynchronously, add tracing.

2018-09-05 Thread Sam McCall via Phabricator via cfe-commits 
sammccall added a comment.

I think you're right - this isn't actually asynchronous (I didn't manage to 
test that!), and if it were it'd interfere with clean shutdown.

I think both issues can be addressed by assigning the future to a variable 
scoped to main. Will send a patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D51638



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51638: [clangd] Load static index asynchronously, add tracing.

2018-09-05 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added inline comments.



Comment at: clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp:287
+StaticIdx.reset(Placeholder = new 
SwapIndex(llvm::make_unique()));
+runAsync([Placeholder] {
+  if (auto Idx = loadIndex(YamlSymbolFile))

Wouldn't the future returned by `runAsync` wait in destructor?



Comment at: clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp:288
+runAsync([Placeholder] {
+  if (auto Idx = loadIndex(YamlSymbolFile))
+Placeholder->reset(std::move(Idx));

What happens if clangd tries to exit before the index is loaded?
Could lead to crashes.


Repository:
  rL LLVM

https://reviews.llvm.org/D51638



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51638: [clangd] Load static index asynchronously, add tracing.

2018-09-04 Thread Sam McCall via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL341376: [clangd] Load static index asynchronously, add 
tracing. (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D51638?vs=163840=163847#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51638

Files:
  clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
  clang-tools-extra/trunk/clangd/index/SymbolYAML.h
  clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp


Index: clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
@@ -281,9 +281,15 @@
   Opts.BuildDynamicSymbolIndex = EnableIndex;
   std::unique_ptr StaticIdx;
   if (EnableIndex && !YamlSymbolFile.empty()) {
-StaticIdx = loadIndex(YamlSymbolFile, UseDex);
-Opts.StaticIndex = StaticIdx.get();
+// Load the index asynchronously. Meanwhile SwapIndex returns no results.
+SwapIndex *Placeholder;
+StaticIdx.reset(Placeholder = new 
SwapIndex(llvm::make_unique()));
+runAsync([Placeholder] {
+  if (auto Idx = loadIndex(YamlSymbolFile))
+Placeholder->reset(std::move(Idx));
+});
   }
+  Opts.StaticIndex = StaticIdx.get();
   Opts.AsyncThreadsCount = WorkerThreadsCount;
 
   clangd::CodeCompleteOptions CCOpts;
Index: clang-tools-extra/trunk/clangd/index/SymbolYAML.h
===
--- clang-tools-extra/trunk/clangd/index/SymbolYAML.h
+++ clang-tools-extra/trunk/clangd/index/SymbolYAML.h
@@ -44,7 +44,7 @@
 // Build an in-memory static index for global symbols from a symbol file.
 // The size of global symbols should be relatively small, so that all symbols
 // can be managed in memory.
-std::unique_ptr loadIndex(llvm::StringRef SymbolFile,
+std::unique_ptr loadIndex(llvm::StringRef SymbolFilename,
bool UseDex = true);
 
 } // namespace clangd
Index: clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
===
--- clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
+++ clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
@@ -8,6 +8,7 @@
 
//===--===//
 
 #include "SymbolYAML.h"
+#include "../Trace.h"
 #include "Index.h"
 #include "Serialization.h"
 #include "dex/DexIndex.h"
@@ -183,25 +184,31 @@
   return OS.str();
 }
 
-std::unique_ptr loadIndex(llvm::StringRef SymbolFile,
+std::unique_ptr loadIndex(llvm::StringRef SymbolFilename,
bool UseDex) {
-  auto Buffer = llvm::MemoryBuffer::getFile(SymbolFile);
+  trace::Span OverallTracer("LoadIndex");
+  auto Buffer = llvm::MemoryBuffer::getFile(SymbolFilename);
   if (!Buffer) {
-llvm::errs() << "Can't open " << SymbolFile << "\n";
+llvm::errs() << "Can't open " << SymbolFilename << "\n";
 return nullptr;
   }
   StringRef Data = Buffer->get()->getBuffer();
 
   llvm::Optional Slab;
   if (Data.startswith("RIFF")) { // Magic for binary index file.
+trace::Span Tracer("ParseRIFF");
 if (auto RIFF = readIndexFile(Data))
   Slab = std::move(RIFF->Symbols);
 else
   llvm::errs() << "Bad RIFF: " << llvm::toString(RIFF.takeError()) << "\n";
   } else {
+trace::Span Tracer("ParseYAML");
 Slab = symbolsFromYAML(Data);
   }
 
+  if (!Slab)
+return nullptr;
+  trace::Span Tracer("BuildIndex");
   return UseDex ? dex::DexIndex::build(std::move(*Slab))
 : MemIndex::build(std::move(*Slab), RefSlab());
 }


Index: clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
@@ -281,9 +281,15 @@
   Opts.BuildDynamicSymbolIndex = EnableIndex;
   std::unique_ptr StaticIdx;
   if (EnableIndex && !YamlSymbolFile.empty()) {
-StaticIdx = loadIndex(YamlSymbolFile, UseDex);
-Opts.StaticIndex = StaticIdx.get();
+// Load the index asynchronously. Meanwhile SwapIndex returns no results.
+SwapIndex *Placeholder;
+StaticIdx.reset(Placeholder = new SwapIndex(llvm::make_unique()));
+runAsync([Placeholder] {
+  if (auto Idx = loadIndex(YamlSymbolFile))
+Placeholder->reset(std::move(Idx));
+});
   }
+  Opts.StaticIndex = StaticIdx.get();
   Opts.AsyncThreadsCount = WorkerThreadsCount;
 
   clangd::CodeCompleteOptions CCOpts;
Index: clang-tools-extra/trunk/clangd/index/SymbolYAML.h
===
--- clang-tools-extra/trunk/clangd/index/SymbolYAML.h
+++ clang-tools-extra/trunk/clangd/index/SymbolYAML.h
@@ -44,7 +44,7 @@
 // Build an in-memory static index for global symbols from a symbol

[PATCH] D51638: [clangd] Load static index asynchronously, add tracing.

2018-09-04 Thread Eric Liu via Phabricator via cfe-commits 
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.

lg. Thanks!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51638



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51638: [clangd] Load static index asynchronously, add tracing.

2018-09-04 Thread Sam McCall via Phabricator via cfe-commits 
sammccall created this revision.
sammccall added a reviewer: ioeric.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ilya-biryukov.

Like https://reviews.llvm.org/D51475 but simplified based on recent patches.
While here, clarify that loadIndex() takes a filename, not file content.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51638

Files:
  clangd/index/SymbolYAML.cpp
  clangd/index/SymbolYAML.h
  clangd/tool/ClangdMain.cpp


Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -280,9 +280,15 @@
   Opts.BuildDynamicSymbolIndex = EnableIndex;
   std::unique_ptr StaticIdx;
   if (EnableIndex && !YamlSymbolFile.empty()) {
-StaticIdx = loadIndex(YamlSymbolFile, UseDex);
-Opts.StaticIndex = StaticIdx.get();
+// Load the index asynchronously. Meanwhile SwapIndex returns no results.
+SwapIndex *Placeholder;
+StaticIdx.reset(Placeholder = new 
SwapIndex(llvm::make_unique()));
+runAsync([Placeholder] {
+  if (auto Idx = loadIndex(YamlSymbolFile))
+Placeholder->reset(std::move(Idx));
+});
   }
+  Opts.StaticIndex = StaticIdx.get();
   Opts.AsyncThreadsCount = WorkerThreadsCount;
 
   clangd::CodeCompleteOptions CCOpts;
Index: clangd/index/SymbolYAML.h
===
--- clangd/index/SymbolYAML.h
+++ clangd/index/SymbolYAML.h
@@ -44,7 +44,7 @@
 // Build an in-memory static index for global symbols from a symbol file.
 // The size of global symbols should be relatively small, so that all symbols
 // can be managed in memory.
-std::unique_ptr loadIndex(llvm::StringRef SymbolFile,
+std::unique_ptr loadIndex(llvm::StringRef SymbolFilename,
bool UseDex = true);
 
 } // namespace clangd
Index: clangd/index/SymbolYAML.cpp
===
--- clangd/index/SymbolYAML.cpp
+++ clangd/index/SymbolYAML.cpp
@@ -8,6 +8,7 @@
 
//===--===//
 
 #include "SymbolYAML.h"
+#include "../Trace.h"
 #include "Index.h"
 #include "dex/DexIndex.h"
 #include "llvm/ADT/Optional.h"
@@ -182,17 +183,24 @@
   return OS.str();
 }
 
-std::unique_ptr loadIndex(llvm::StringRef SymbolFile,
+std::unique_ptr loadIndex(llvm::StringRef SymbolFilename,
bool UseDex) {
-  auto Buffer = llvm::MemoryBuffer::getFile(SymbolFile);
+  trace::Span OverallTracer("LoadIndex");
+  auto Buffer = llvm::MemoryBuffer::getFile(SymbolFilename);
   if (!Buffer) {
-llvm::errs() << "Can't open " << SymbolFile << "\n";
+llvm::errs() << "Can't open " << SymbolFilename << "\n";
 return nullptr;
   }
-  auto Slab = symbolsFromYAML(Buffer.get()->getBuffer());
+  StringRef Data = Buffer.get()->getBuffer();
+  llvm::Optional SymbolSlab;
+  {
+trace::Span Tracer("ParseYAML");
+auto Slab = symbolsFromYAML(Data);
+  }
 
-  return UseDex ? dex::DexIndex::build(std::move(Slab))
-: MemIndex::build(std::move(Slab), RefSlab());
+  trace::Span Tracer("BuildIndex");
+  return UseDex ? dex::DexIndex::build(std::move(*SymbolSlab))
+: MemIndex::build(std::move(*SymbolSlab), RefSlab());
 }
 
 } // namespace clangd


Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -280,9 +280,15 @@
   Opts.BuildDynamicSymbolIndex = EnableIndex;
   std::unique_ptr StaticIdx;
   if (EnableIndex && !YamlSymbolFile.empty()) {
-StaticIdx = loadIndex(YamlSymbolFile, UseDex);
-Opts.StaticIndex = StaticIdx.get();
+// Load the index asynchronously. Meanwhile SwapIndex returns no results.
+SwapIndex *Placeholder;
+StaticIdx.reset(Placeholder = new SwapIndex(llvm::make_unique()));
+runAsync([Placeholder] {
+  if (auto Idx = loadIndex(YamlSymbolFile))
+Placeholder->reset(std::move(Idx));
+});
   }
+  Opts.StaticIndex = StaticIdx.get();
   Opts.AsyncThreadsCount = WorkerThreadsCount;
 
   clangd::CodeCompleteOptions CCOpts;
Index: clangd/index/SymbolYAML.h
===
--- clangd/index/SymbolYAML.h
+++ clangd/index/SymbolYAML.h
@@ -44,7 +44,7 @@
 // Build an in-memory static index for global symbols from a symbol file.
 // The size of global symbols should be relatively small, so that all symbols
 // can be managed in memory.
-std::unique_ptr loadIndex(llvm::StringRef SymbolFile,
+std::unique_ptr loadIndex(llvm::StringRef SymbolFilename,
bool UseDex = true);
 
 } // namespace clangd
Index: clangd/index/SymbolYAML.cpp
===
--- clangd/index/SymbolYAML.cpp
+++ clangd/index/SymbolYAML.cpp
@@ -8,6 +8,7 @@