Query the StringMap only once when creating MDString (NFC)

Summary: Loading IR with debug info improves MDString::get() from 19ms to 10ms. This is a rework of D16597 with adding an "emplace" method on the StringMap to avoid requiring the MDString move ctor to be public. Reviewers: dexonsmith Subscribers: llvm-commits Differential Revision: http://reviews.llvm.org/D17920 From: Mehdi Amini <mehdi.amini@apple.com> llvm-svn: 264386
llvm · Mar 25, 2016 · cb708b2 · cb708b2
1 parent be8a57f
commit cb708b2
Showing 4 changed files with 74 additions and 41 deletions.
diff --git a/llvm/include/llvm/ADT/StringMap.h b/llvm/include/llvm/ADT/StringMap.h
@@ -143,11 +143,11 @@ class StringMapEntry : public StringMapEntryBase {
 
   StringRef first() const { return StringRef(getKeyData(), getKeyLength()); }
 
-  /// Create - Create a StringMapEntry for the specified key and default
-  /// construct the value.
-  template <typename AllocatorTy, typename InitType>
+  /// Create a StringMapEntry for the specified key construct the value using
+  /// \p InitiVals.
+  template <typename AllocatorTy, typename... InitTypes>
   static StringMapEntry *Create(StringRef Key, AllocatorTy &Allocator,
-                                InitType &&InitVal) {
+                                InitTypes &&... InitVals) {
     unsigned KeyLength = Key.size();
 
     // Allocate a new item with space for the string at the end and a null
@@ -159,8 +159,9 @@ class StringMapEntry : public StringMapEntryBase {
     StringMapEntry *NewItem =
       static_cast<StringMapEntry*>(Allocator.Allocate(AllocSize,Alignment));
 
-    // Default construct the value.
-    new (NewItem) StringMapEntry(KeyLength, std::forward<InitType>(InitVal));
+    // Construct the value.
+    new (NewItem)
+        StringMapEntry(KeyLength, std::forward<InitTypes>(InitVals)...);
 
     // Copy the string information.
     char *StrBuffer = const_cast<char*>(NewItem->getKeyData());
@@ -170,11 +171,6 @@ class StringMapEntry : public StringMapEntryBase {
     return NewItem;
   }
 
-  template<typename AllocatorTy>
-  static StringMapEntry *Create(StringRef Key, AllocatorTy &Allocator) {
-    return Create(Key, Allocator, ValueTy());
-  }
-
   /// Create - Create a StringMapEntry with normal malloc/free.
   template<typename InitType>
   static StringMapEntry *Create(StringRef Key, InitType &&InitVal) {
@@ -296,8 +292,10 @@ class StringMap : public StringMapImpl {
     return ValueTy();
   }
 
+  /// Lookup the ValueTy for the \p Key, or create a default constructed value
+  /// if the key is not in the map.
   ValueTy &operator[](StringRef Key) {
-    return insert(std::make_pair(Key, ValueTy())).first->second;
+    return emplace_second(Key).first->second;
   }
 
   /// count - Return 1 if the element is in the map, 0 otherwise.
@@ -329,16 +327,24 @@ class StringMap : public StringMapImpl {
   /// if and only if the insertion takes place, and the iterator component of
   /// the pair points to the element with key equivalent to the key of the pair.
   std::pair<iterator, bool> insert(std::pair<StringRef, ValueTy> KV) {
-    unsigned BucketNo = LookupBucketFor(KV.first);
+    return emplace_second(KV.first, std::move(KV.second));
+  }
+
+  /// Emplace a new element for the specified key into the map if the key isn't
+  /// already in the map. The bool component of the returned pair is true
+  /// if and only if the insertion takes place, and the iterator component of
+  /// the pair points to the element with key equivalent to the key of the pair.
+  template <typename... ArgsTy>
+  std::pair<iterator, bool> emplace_second(StringRef Key, ArgsTy &&... Args) {
+    unsigned BucketNo = LookupBucketFor(Key);
     StringMapEntryBase *&Bucket = TheTable[BucketNo];
     if (Bucket && Bucket != getTombstoneVal())
       return std::make_pair(iterator(TheTable + BucketNo, false),
                             false); // Already exists in map.
 
     if (Bucket == getTombstoneVal())
       --NumTombstones;
-    Bucket =
-        MapEntryTy::Create(KV.first, Allocator, std::move(KV.second));
+    Bucket = MapEntryTy::Create(Key, Allocator, std::forward<ArgsTy>(Args)...);
     ++NumItems;
     assert(NumItems + NumTombstones <= NumBuckets);
 

diff --git a/llvm/include/llvm/IR/Metadata.h b/llvm/include/llvm/IR/Metadata.h
@@ -592,7 +592,6 @@ class MDString : public Metadata {
 
   StringMapEntry<MDString> *Entry;
   MDString() : Metadata(MDStringKind, Uniqued), Entry(nullptr) {}
-  MDString(MDString &&) : Metadata(MDStringKind, Uniqued) {}
 
 public:
   static MDString *get(LLVMContext &Context, StringRef Str);

diff --git a/llvm/lib/IR/Metadata.cpp b/llvm/lib/IR/Metadata.cpp
@@ -397,17 +397,12 @@ void ValueAsMetadata::handleRAUW(Value *From, Value *To) {
 
 MDString *MDString::get(LLVMContext &Context, StringRef Str) {
   auto &Store = Context.pImpl->MDStringCache;
-  auto I = Store.find(Str);
-  if (I != Store.end())
-    return &I->second;
-
-  auto *Entry =
-      StringMapEntry<MDString>::Create(Str, Store.getAllocator(), MDString());
-  bool WasInserted = Store.insert(Entry);
-  (void)WasInserted;
-  assert(WasInserted && "Expected entry to be inserted");
-  Entry->second.Entry = Entry;
-  return &Entry->second;
+  auto I = Store.emplace_second(Str);
+  auto &MapEntry = I.first->getValue();
+  if (!I.second)
+    return &MapEntry;
+  MapEntry.Entry = &*I.first;
+  return &MapEntry;
 }
 
 StringRef MDString::getString() const {

diff --git a/llvm/unittests/ADT/StringMapTest.cpp b/llvm/unittests/ADT/StringMapTest.cpp
@@ -358,24 +358,28 @@ TEST_F(StringMapTest, MoveDtor) {
 
 namespace {
 // Simple class that counts how many moves and copy happens when growing a map
-struct CountCopyAndMove {
+struct CountCtorCopyAndMove {
+  static unsigned Ctor;
   static unsigned Move;
   static unsigned Copy;
-  CountCopyAndMove() {}
+  int Data = 0;
+  CountCtorCopyAndMove(int Data) : Data(Data) { Ctor++; }
+  CountCtorCopyAndMove() { Ctor++; }
 
-  CountCopyAndMove(const CountCopyAndMove &) { Copy++; }
-  CountCopyAndMove &operator=(const CountCopyAndMove &) {
+  CountCtorCopyAndMove(const CountCtorCopyAndMove &) { Copy++; }
+  CountCtorCopyAndMove &operator=(const CountCtorCopyAndMove &) {
     Copy++;
     return *this;
   }
-  CountCopyAndMove(CountCopyAndMove &&) { Move++; }
-  CountCopyAndMove &operator=(const CountCopyAndMove &&) {
+  CountCtorCopyAndMove(CountCtorCopyAndMove &&) { Move++; }
+  CountCtorCopyAndMove &operator=(const CountCtorCopyAndMove &&) {
     Move++;
     return *this;
   }
 };
-unsigned CountCopyAndMove::Copy = 0;
-unsigned CountCopyAndMove::Move = 0;
+unsigned CountCtorCopyAndMove::Copy = 0;
+unsigned CountCtorCopyAndMove::Move = 0;
+unsigned CountCtorCopyAndMove::Ctor = 0;
 
 } // anonymous namespace
 
@@ -385,14 +389,43 @@ TEST(StringMapCustomTest, InitialSizeTest) {
   // 1 is an "edge value", 32 is an arbitrary power of two, and 67 is an
   // arbitrary prime, picked without any good reason.
   for (auto Size : {1, 32, 67}) {
-    StringMap<CountCopyAndMove> Map(Size);
-    CountCopyAndMove::Copy = 0;
-    CountCopyAndMove::Move = 0;
+    StringMap<CountCtorCopyAndMove> Map(Size);
+    CountCtorCopyAndMove::Move = 0;
+    CountCtorCopyAndMove::Copy = 0;
     for (int i = 0; i < Size; ++i)
-      Map.insert(std::make_pair(Twine(i).str(), CountCopyAndMove()));
-    EXPECT_EQ((unsigned)Size * 3, CountCopyAndMove::Move);
-    EXPECT_EQ(0u, CountCopyAndMove::Copy);
+      Map.insert(std::make_pair(Twine(i).str(), CountCtorCopyAndMove()));
+    EXPECT_EQ((unsigned)Size * 3, CountCtorCopyAndMove::Move);
+    EXPECT_EQ(0u, CountCtorCopyAndMove::Copy);
   }
 }
 
+TEST(StringMapCustomTest, BracketOperatorCtor) {
+  StringMap<CountCtorCopyAndMove> Map;
+  CountCtorCopyAndMove::Ctor = 0;
+  Map["abcd"];
+  EXPECT_EQ(1u, CountCtorCopyAndMove::Ctor);
+  // Test that operator[] does not create a value when it is already in the map
+  CountCtorCopyAndMove::Ctor = 0;
+  Map["abcd"];
+  EXPECT_EQ(0u, CountCtorCopyAndMove::Ctor);
+}
+
+namespace {
+struct NonMoveableNonCopyableType {
+  int Data = 0;
+  NonMoveableNonCopyableType() = default;
+  NonMoveableNonCopyableType(int Data) : Data(Data) {}
+  NonMoveableNonCopyableType(const NonMoveableNonCopyableType &) = delete;
+  NonMoveableNonCopyableType(NonMoveableNonCopyableType &&) = delete;
+};
+}
+
+// Test that we can "emplace" an element in the map without involving map/move
+TEST(StringMapCustomTest, EmplaceTest) {
+  StringMap<NonMoveableNonCopyableType> Map;
+  Map.emplace_second("abcd", 42);
+  EXPECT_EQ(1u, Map.count("abcd"));
+  EXPECT_EQ(42, Map["abcd"].Data);
+}
+
 } // end anonymous namespace