[llvm] r264386 - Query the StringMap only once when creating MDString (NFC)
Mehdi Amini via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 24 22:58:04 PDT 2016
Author: mehdi_amini
Date: Fri Mar 25 00:58:04 2016
New Revision: 264386
URL: http://llvm.org/viewvc/llvm-project?rev=264386&view=rev
Log:
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 at apple.com>
Modified:
llvm/trunk/include/llvm/ADT/StringMap.h
llvm/trunk/include/llvm/IR/Metadata.h
llvm/trunk/lib/IR/Metadata.cpp
llvm/trunk/unittests/ADT/StringMapTest.cpp
Modified: llvm/trunk/include/llvm/ADT/StringMap.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringMap.h?rev=264386&r1=264385&r2=264386&view=diff
==============================================================================
--- llvm/trunk/include/llvm/ADT/StringMap.h (original)
+++ llvm/trunk/include/llvm/ADT/StringMap.h Fri Mar 25 00:58:04 2016
@@ -143,11 +143,11 @@ public:
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 @@ public:
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 @@ public:
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 @@ public:
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,7 +327,16 @@ public:
/// 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),
@@ -337,8 +344,7 @@ public:
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);
Modified: llvm/trunk/include/llvm/IR/Metadata.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Metadata.h?rev=264386&r1=264385&r2=264386&view=diff
==============================================================================
--- llvm/trunk/include/llvm/IR/Metadata.h (original)
+++ llvm/trunk/include/llvm/IR/Metadata.h Fri Mar 25 00:58:04 2016
@@ -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);
Modified: llvm/trunk/lib/IR/Metadata.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Metadata.cpp?rev=264386&r1=264385&r2=264386&view=diff
==============================================================================
--- llvm/trunk/lib/IR/Metadata.cpp (original)
+++ llvm/trunk/lib/IR/Metadata.cpp Fri Mar 25 00:58:04 2016
@@ -397,17 +397,12 @@ void ValueAsMetadata::handleRAUW(Value *
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 {
Modified: llvm/trunk/unittests/ADT/StringMapTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/StringMapTest.cpp?rev=264386&r1=264385&r2=264386&view=diff
==============================================================================
--- llvm/trunk/unittests/ADT/StringMapTest.cpp (original)
+++ llvm/trunk/unittests/ADT/StringMapTest.cpp Fri Mar 25 00:58:04 2016
@@ -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, InitialSizeTes
// 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
More information about the llvm-commits
mailing list