[PATCH] D45479: [Tooling] Optimize memory usage in InMemoryToolResults.

2018-04-11 Thread Haojian Wu via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rC329784: [Tooling] Optimize memory usage in 
InMemoryToolResults. (authored by hokein, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D45479?vs=141842=141956#toc

Repository:
  rC Clang

https://reviews.llvm.org/D45479

Files:
  include/clang/Tooling/Execution.h
  lib/Tooling/AllTUsExecution.cpp
  lib/Tooling/Execution.cpp


Index: lib/Tooling/Execution.cpp
===
--- lib/Tooling/Execution.cpp
+++ lib/Tooling/Execution.cpp
@@ -21,10 +21,19 @@
  llvm::cl::init("standalone"));
 
 void InMemoryToolResults::addResult(StringRef Key, StringRef Value) {
-  KVResults.push_back({Key.str(), Value.str()});
+  auto Intern = [&](StringRef ) {
+auto R = Strings.insert(V);
+if (R.second) { // A new entry, create a new string copy.
+  *R.first = StringsPool.save(V);
+}
+V = *R.first;
+  };
+  Intern(Key);
+  Intern(Value);
+  KVResults.push_back({Key, Value});
 }
 
-std::vector>
+std::vector>
 InMemoryToolResults::AllKVResults() {
   return KVResults;
 }
Index: lib/Tooling/AllTUsExecution.cpp
===
--- lib/Tooling/AllTUsExecution.cpp
+++ lib/Tooling/AllTUsExecution.cpp
@@ -36,7 +36,8 @@
 Results.addResult(Key, Value);
   }
 
-  std::vector> AllKVResults() override {
+  std::vector>
+  AllKVResults() override {
 return Results.AllKVResults();
   }
 
Index: include/clang/Tooling/Execution.h
===
--- include/clang/Tooling/Execution.h
+++ include/clang/Tooling/Execution.h
@@ -32,6 +32,7 @@
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Registry.h"
+#include "llvm/Support/StringSaver.h"
 
 namespace clang {
 namespace tooling {
@@ -45,20 +46,30 @@
 public:
   virtual ~ToolResults() = default;
   virtual void addResult(StringRef Key, StringRef Value) = 0;
-  virtual std::vector> AllKVResults() = 0;
+  virtual std::vector>
+  AllKVResults() = 0;
   virtual void forEachResult(
   llvm::function_ref Callback) = 0;
 };
 
+/// \brief Stores the key-value results in memory. It maintains the lifetime of
+/// the result. Clang tools using this class are expected to generate a small
+/// set of different results, or a large set of duplicated results.
 class InMemoryToolResults : public ToolResults {
 public:
+  InMemoryToolResults() : StringsPool(Arena) {}
   void addResult(StringRef Key, StringRef Value) override;
-  std::vector> AllKVResults() override;
+  std::vector>
+  AllKVResults() override;
   void forEachResult(llvm::function_ref
  Callback) override;
 
 private:
-  std::vector> KVResults;
+  llvm::BumpPtrAllocator Arena;
+  llvm::StringSaver StringsPool;
+  llvm::DenseSet Strings;
+
+  std::vector> KVResults;
 };
 
 /// \brief The context of an execution, including the information about


Index: lib/Tooling/Execution.cpp
===
--- lib/Tooling/Execution.cpp
+++ lib/Tooling/Execution.cpp
@@ -21,10 +21,19 @@
  llvm::cl::init("standalone"));
 
 void InMemoryToolResults::addResult(StringRef Key, StringRef Value) {
-  KVResults.push_back({Key.str(), Value.str()});
+  auto Intern = [&](StringRef ) {
+auto R = Strings.insert(V);
+if (R.second) { // A new entry, create a new string copy.
+  *R.first = StringsPool.save(V);
+}
+V = *R.first;
+  };
+  Intern(Key);
+  Intern(Value);
+  KVResults.push_back({Key, Value});
 }
 
-std::vector>
+std::vector>
 InMemoryToolResults::AllKVResults() {
   return KVResults;
 }
Index: lib/Tooling/AllTUsExecution.cpp
===
--- lib/Tooling/AllTUsExecution.cpp
+++ lib/Tooling/AllTUsExecution.cpp
@@ -36,7 +36,8 @@
 Results.addResult(Key, Value);
   }
 
-  std::vector> AllKVResults() override {
+  std::vector>
+  AllKVResults() override {
 return Results.AllKVResults();
   }
 
Index: include/clang/Tooling/Execution.h
===
--- include/clang/Tooling/Execution.h
+++ include/clang/Tooling/Execution.h
@@ -32,6 +32,7 @@
 #include

[PATCH] D45479: [Tooling] Optimize memory usage in InMemoryToolResults.

2018-04-10 Thread Haojian Wu via Phabricator via cfe-commits 
hokein updated this revision to Diff 141842.
hokein edited the summary of this revision.
hokein added a comment.

Add a comment.


Repository:
  rC Clang

https://reviews.llvm.org/D45479

Files:
  include/clang/Tooling/Execution.h
  lib/Tooling/AllTUsExecution.cpp
  lib/Tooling/Execution.cpp


Index: lib/Tooling/Execution.cpp
===
--- lib/Tooling/Execution.cpp
+++ lib/Tooling/Execution.cpp
@@ -21,10 +21,19 @@
  llvm::cl::init("standalone"));
 
 void InMemoryToolResults::addResult(StringRef Key, StringRef Value) {
-  KVResults.push_back({Key.str(), Value.str()});
+  auto Intern = [&](StringRef ) {
+auto R = Strings.insert(V);
+if (R.second) { // A new entry, create a new string copy.
+  *R.first = StringsPool.save(V);
+}
+V = *R.first;
+  };
+  Intern(Key);
+  Intern(Value);
+  KVResults.push_back({Key, Value});
 }
 
-std::vector>
+std::vector>
 InMemoryToolResults::AllKVResults() {
   return KVResults;
 }
Index: lib/Tooling/AllTUsExecution.cpp
===
--- lib/Tooling/AllTUsExecution.cpp
+++ lib/Tooling/AllTUsExecution.cpp
@@ -36,7 +36,8 @@
 Results.addResult(Key, Value);
   }
 
-  std::vector> AllKVResults() override {
+  std::vector>
+  AllKVResults() override {
 return Results.AllKVResults();
   }
 
Index: include/clang/Tooling/Execution.h
===
--- include/clang/Tooling/Execution.h
+++ include/clang/Tooling/Execution.h
@@ -32,6 +32,7 @@
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Registry.h"
+#include "llvm/Support/StringSaver.h"
 
 namespace clang {
 namespace tooling {
@@ -45,20 +46,30 @@
 public:
   virtual ~ToolResults() = default;
   virtual void addResult(StringRef Key, StringRef Value) = 0;
-  virtual std::vector> AllKVResults() = 0;
+  virtual std::vector>
+  AllKVResults() = 0;
   virtual void forEachResult(
   llvm::function_ref Callback) = 0;
 };
 
+/// \brief Stores the key-value results in memory. It maintains the lifetime of
+/// the result. Clang tools using this class are expected to generate a small
+/// set of different results, or a large set of duplicated results.
 class InMemoryToolResults : public ToolResults {
 public:
+  InMemoryToolResults() : StringsPool(Arena) {}
   void addResult(StringRef Key, StringRef Value) override;
-  std::vector> AllKVResults() override;
+  std::vector>
+  AllKVResults() override;
   void forEachResult(llvm::function_ref
  Callback) override;
 
 private:
-  std::vector> KVResults;
+  llvm::BumpPtrAllocator Arena;
+  llvm::StringSaver StringsPool;
+  llvm::DenseSet Strings;
+
+  std::vector> KVResults;
 };
 
 /// \brief The context of an execution, including the information about


Index: lib/Tooling/Execution.cpp
===
--- lib/Tooling/Execution.cpp
+++ lib/Tooling/Execution.cpp
@@ -21,10 +21,19 @@
  llvm::cl::init("standalone"));
 
 void InMemoryToolResults::addResult(StringRef Key, StringRef Value) {
-  KVResults.push_back({Key.str(), Value.str()});
+  auto Intern = [&](StringRef ) {
+auto R = Strings.insert(V);
+if (R.second) { // A new entry, create a new string copy.
+  *R.first = StringsPool.save(V);
+}
+V = *R.first;
+  };
+  Intern(Key);
+  Intern(Value);
+  KVResults.push_back({Key, Value});
 }
 
-std::vector>
+std::vector>
 InMemoryToolResults::AllKVResults() {
   return KVResults;
 }
Index: lib/Tooling/AllTUsExecution.cpp
===
--- lib/Tooling/AllTUsExecution.cpp
+++ lib/Tooling/AllTUsExecution.cpp
@@ -36,7 +36,8 @@
 Results.addResult(Key, Value);
   }
 
-  std::vector> AllKVResults() override {
+  std::vector>
+  AllKVResults() override {
 return Results.AllKVResults();
   }
 
Index: include/clang/Tooling/Execution.h
===
--- include/clang/Tooling/Execution.h
+++ include/clang/Tooling/Execution.h
@@ -32,6 +32,7 @@
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Registry.h"
+#include "llvm/Support/StringSaver.h"
 
 namespace clang {
 namespace

[PATCH] D45479: [Tooling] Optimize memory usage in InMemoryToolResults.

2018-04-10 Thread Sam McCall via Phabricator via cfe-commits 
sammccall added inline comments.



Comment at: include/clang/Tooling/Execution.h:55
 
 class InMemoryToolResults : public ToolResults {
 public:

please add a high-level comment explaining what this is caching and when we 
expect it to work well.



Repository:
  rC Clang

https://reviews.llvm.org/D45479



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

[PATCH] D45479: [Tooling] Optimize memory usage in InMemoryToolResults.

2018-04-10 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added a comment.

This definitely helps with memory consumption of clangd's 
global-symbol-indexer, I can now run it over LLVM on MacBook without it 
swapping out of RAM. The physical memory usage goes up to 3GB at some point, 
before it was > 20GB.

Strictly speaking, interning the strings is probably more work for some 
workloads, so I'm not sure it's a better in all cases.
On average this LG, though, so LGTM from my side.


Repository:
  rC Clang

https://reviews.llvm.org/D45479



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