Parallelize istring creation (#1008)

* parallelize istring creation, by having a thread-local set and a global set guarded by a mutex. each time a new string shows up in a thread, it will be added to that thread's set, after accessing the global set through the lock first, which means we lock at most once per new string per thread

* don't leak strings in istring store

* since we now create names in a parallel thread-safe manner, we don't need to pre-create names in RelooperJumpThreading

2 files changed, 37 insertions, 42 deletions

diff --git a/src/emscripten-optimizer/istring.h b/src/emscripten-optimizer/istring.h
index e47361eeb..b991159e5 100644
--- a/src/emscripten-optimizer/istring.h
+++ b/src/emscripten-optimizer/istring.h
@@ -30,6 +30,7 @@
 #include <assert.h>
 
 #include "support/threads.h"
+#include "support/utilities.h"
 
 namespace cashew {
 
@@ -66,21 +67,35 @@ struct IString {
 
   void set(const char *s, bool reuse=true) {
     typedef std::unordered_set<const char *, CStringHash, CStringEqual> StringSet;
-    static StringSet* strings = new StringSet();
-
-    auto existing = strings->find(s);
-
-    if (existing == strings->end()) {
-      // the StringSet cache is a global shared structure, which should
-      // not be modified by multiple threads at once.
-      assert(!wasm::ThreadPool::isRunning());
-      if (!reuse) {
-        size_t len = strlen(s) + 1;
-        char *copy = (char*)malloc(len); // XXX leaked
-        strncpy(copy, s, len);
-        s = copy;
+    // one global store of strings per thread, we must not access this
+    // in parallel
+    thread_local static StringSet strings;
+
+    auto existing = strings.find(s);
+
+    if (existing == strings.end()) {
+      // if the string isn't already known, we must use a single global
+      // storage location, guarded by a mutex, so each string is allocated
+      // exactly once
+      static std::mutex mutex;
+      std::unique_lock<std::mutex> lock(mutex);
+      // a single global set contains the actual strings, so we allocate each one
+      // exactly once.
+      static StringSet globalStrings;
+      auto globalExisting = globalStrings.find(s);
+      if (globalExisting == globalStrings.end()) {
+        if (!reuse) {
+          static std::vector<std::unique_ptr<std::string>> allocated;
+          allocated.emplace_back(wasm::make_unique<std::string>(s));
+          s = allocated.back()->c_str(); // we'll never modify it, so this is ok
+        }
+        // insert into global set
+        globalStrings.insert(s);
+      } else {
+        s = *globalExisting;
       }
-      strings->insert(s);
+      // add the string to our thread-local set
+      strings.insert(s);
     } else {
       s = *existing;
     }
diff --git a/src/passes/RelooperJumpThreading.cpp b/src/passes/RelooperJumpThreading.cpp
index 9a6d82e0e..d3fd1844f 100644
--- a/src/passes/RelooperJumpThreading.cpp
+++ b/src/passes/RelooperJumpThreading.cpp
@@ -27,25 +27,13 @@ namespace wasm {
 
 static Name LABEL("label");
 
-// We need to use new label names, which we cannot create in parallel, so pre-create them
-
-const Index MAX_NAME_INDEX = 10000;
-
-std::vector<Name>* innerNames = nullptr;
-std::vector<Name>* outerNames = nullptr;
+static Name getInnerName(int i) {
+  return Name(std::string("__rjti$") + std::to_string(i));
+}
 
-struct NameEnsurer {
-  NameEnsurer() {
-    assert(!innerNames);
-    assert(!outerNames);
-    innerNames = new std::vector<Name>;
-    outerNames = new std::vector<Name>;
-    for (Index i = 0; i < MAX_NAME_INDEX; i++) {
-      innerNames->push_back(Name(std::string("__rjti$") + std::to_string(i)));
-      outerNames->push_back(Name(std::string("__rjto$") + std::to_string(i)));
-    }
-  }
-};
+static Name getOuterName(int i) {
+  return Name(std::string("__rjto$") + std::to_string(i));
+}
 
 static If* isLabelCheckingIf(Expression* curr, Index labelIndex) {
   if (!curr) return nullptr;
@@ -99,10 +87,6 @@ struct RelooperJumpThreading : public WalkerPass<ExpressionStackWalker<RelooperJ
 
   Pass* create() override { return new RelooperJumpThreading; }
 
-  void prepareToRun(PassRunner* runner, Module* module) override {
-    static NameEnsurer ensurer;
-  }
-
   std::map<Index, Index> labelChecks;
   std::map<Index, Index> labelSets;
 
@@ -208,17 +192,13 @@ private:
   //  * iff is the if
   void optimizeJumpsToLabelCheck(Expression*& origin, If* iff) {
     Index nameCounter = newNameCounter++;
-    if (nameCounter >= MAX_NAME_INDEX) {
-      std::cerr << "too many names in RelooperJumpThreading :(\n";
-      return;
-    }
     Index num = getCheckedLabelValue(iff);
     // create a new block for this jump target
     Builder builder(*getModule());
     // origin is where all jumps to this target must come from - the element right before this if
     // we break out of inner to reach the target. instead of flowing out of normally, we break out of the outer, so we skip the target.
-    auto innerName = innerNames->at(nameCounter);
-    auto outerName = outerNames->at(nameCounter);
+    auto innerName = getInnerName(nameCounter);
+    auto outerName = getOuterName(nameCounter);
     auto* ifFalse = iff->ifFalse;
     // all assignments of label to the target can be replaced with breaks to the target, via innerName
     struct JumpUpdater : public PostWalker<JumpUpdater> {