[llvm] r365974 - PDB HashTable: Move TraitsT from class parameter to the methods that need it
Nico Weber via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 12 16:30:56 PDT 2019
Author: nico
Date: Fri Jul 12 16:30:55 2019
New Revision: 365974
URL: http://llvm.org/viewvc/llvm-project?rev=365974&view=rev
Log:
PDB HashTable: Move TraitsT from class parameter to the methods that need it
The traits object is only used by a few methods. Deserializing a hash
table and walking it is possible without the traits object, so it
shouldn't be required to build a dummy object for that use case.
The TraitsT object used to be a function template parameter before
r327647, this restores it to that state.
This makes it clear that the traits object isn't needed at all in 1 of
the current 3 uses of HashTable (and I am going to add another use that
doesn't need it), and that the default PdbHashTraits isn't used outside
of tests.
While here, also re-enable 3 checks in the test that were commented out
(which requires making HashTableInternals templated and giving FooBar
an operator==).
No intended behavior change.
Differential Revision: https://reviews.llvm.org/D64640
Modified:
llvm/trunk/include/llvm/DebugInfo/PDB/Native/HashTable.h
llvm/trunk/include/llvm/DebugInfo/PDB/Native/NamedStreamMap.h
llvm/trunk/include/llvm/DebugInfo/PDB/Native/PDBFileBuilder.h
llvm/trunk/lib/DebugInfo/PDB/Native/NamedStreamMap.cpp
llvm/trunk/lib/DebugInfo/PDB/Native/PDBFileBuilder.cpp
llvm/trunk/unittests/DebugInfo/PDB/HashTableTest.cpp
Modified: llvm/trunk/include/llvm/DebugInfo/PDB/Native/HashTable.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo/PDB/Native/HashTable.h?rev=365974&r1=365973&r2=365974&view=diff
==============================================================================
--- llvm/trunk/include/llvm/DebugInfo/PDB/Native/HashTable.h (original)
+++ llvm/trunk/include/llvm/DebugInfo/PDB/Native/HashTable.h Fri Jul 12 16:30:55 2019
@@ -31,21 +31,21 @@ namespace pdb {
Error readSparseBitVector(BinaryStreamReader &Stream, SparseBitVector<> &V);
Error writeSparseBitVector(BinaryStreamWriter &Writer, SparseBitVector<> &Vec);
-template <typename ValueT, typename TraitsT> class HashTable;
+template <typename ValueT> class HashTable;
-template <typename ValueT, typename TraitsT>
+template <typename ValueT>
class HashTableIterator
- : public iterator_facade_base<HashTableIterator<ValueT, TraitsT>,
+ : public iterator_facade_base<HashTableIterator<ValueT>,
std::forward_iterator_tag,
std::pair<uint32_t, ValueT>> {
- friend HashTable<ValueT, TraitsT>;
+ friend HashTable<ValueT>;
- HashTableIterator(const HashTable<ValueT, TraitsT> &Map, uint32_t Index,
+ HashTableIterator(const HashTable<ValueT> &Map, uint32_t Index,
bool IsEnd)
: Map(&Map), Index(Index), IsEnd(IsEnd) {}
public:
- HashTableIterator(const HashTable<ValueT, TraitsT> &Map) : Map(&Map) {
+ HashTableIterator(const HashTable<ValueT> &Map) : Map(&Map) {
int I = Map.Present.find_first();
if (I == -1) {
Index = 0;
@@ -87,22 +87,14 @@ private:
bool isEnd() const { return IsEnd; }
uint32_t index() const { return Index; }
- const HashTable<ValueT, TraitsT> *Map;
+ const HashTable<ValueT> *Map;
uint32_t Index;
bool IsEnd;
};
-template <typename T> struct PdbHashTraits {};
-
-template <> struct PdbHashTraits<uint32_t> {
- uint32_t hashLookupKey(uint32_t N) const { return N; }
- uint32_t storageKeyToLookupKey(uint32_t N) const { return N; }
- uint32_t lookupKeyToStorageKey(uint32_t N) { return N; }
-};
-
-template <typename ValueT, typename TraitsT = PdbHashTraits<ValueT>>
+template <typename ValueT>
class HashTable {
- using iterator = HashTableIterator<ValueT, TraitsT>;
+ using iterator = HashTableIterator<ValueT>;
friend iterator;
struct Header {
@@ -114,9 +106,7 @@ class HashTable {
public:
HashTable() { Buckets.resize(8); }
-
- explicit HashTable(TraitsT Traits) : HashTable(8, std::move(Traits)) {}
- HashTable(uint32_t Capacity, TraitsT Traits) : Traits(Traits) {
+ explicit HashTable(uint32_t Capacity) {
Buckets.resize(Capacity);
}
@@ -221,7 +211,8 @@ public:
/// Find the entry whose key has the specified hash value, using the specified
/// traits defining hash function and equality.
- template <typename Key> iterator find_as(const Key &K) const {
+ template <typename Key, typename TraitsT>
+ iterator find_as(const Key &K, TraitsT &Traits) const {
uint32_t H = Traits.hashLookupKey(K) % capacity();
uint32_t I = H;
Optional<uint32_t> FirstUnused;
@@ -252,12 +243,14 @@ public:
/// Set the entry using a key type that the specified Traits can convert
/// from a real key to an internal key.
- template <typename Key> bool set_as(const Key &K, ValueT V) {
- return set_as_internal(K, std::move(V), None);
+ template <typename Key, typename TraitsT>
+ bool set_as(const Key &K, ValueT V, TraitsT &Traits) {
+ return set_as_internal(K, std::move(V), Traits, None);
}
- template <typename Key> ValueT get(const Key &K) const {
- auto Iter = find_as(K);
+ template <typename Key, typename TraitsT>
+ ValueT get(const Key &K, TraitsT &Traits) const {
+ auto Iter = find_as(K, Traits);
assert(Iter != end());
return (*Iter).second;
}
@@ -266,7 +259,6 @@ protected:
bool isPresent(uint32_t K) const { return Present.test(K); }
bool isDeleted(uint32_t K) const { return Deleted.test(K); }
- TraitsT Traits;
BucketList Buckets;
mutable SparseBitVector<> Present;
mutable SparseBitVector<> Deleted;
@@ -274,9 +266,10 @@ protected:
private:
/// Set the entry using a key type that the specified Traits can convert
/// from a real key to an internal key.
- template <typename Key>
- bool set_as_internal(const Key &K, ValueT V, Optional<uint32_t> InternalKey) {
- auto Entry = find_as(K);
+ template <typename Key, typename TraitsT>
+ bool set_as_internal(const Key &K, ValueT V, TraitsT &Traits,
+ Optional<uint32_t> InternalKey) {
+ auto Entry = find_as(K, Traits);
if (Entry != end()) {
assert(isPresent(Entry.index()));
assert(Traits.storageKeyToLookupKey(Buckets[Entry.index()].first) == K);
@@ -293,15 +286,16 @@ private:
Present.set(Entry.index());
Deleted.reset(Entry.index());
- grow();
+ grow(Traits);
- assert((find_as(K)) != end());
+ assert((find_as(K, Traits)) != end());
return true;
}
static uint32_t maxLoad(uint32_t capacity) { return capacity * 2 / 3 + 1; }
- void grow() {
+ template <typename TraitsT>
+ void grow(TraitsT &Traits) {
uint32_t S = size();
uint32_t MaxLoad = maxLoad(capacity());
if (S < maxLoad(capacity()))
@@ -313,10 +307,11 @@ private:
// Growing requires rebuilding the table and re-hashing every item. Make a
// copy with a larger capacity, insert everything into the copy, then swap
// it in.
- HashTable NewMap(NewCapacity, Traits);
+ HashTable NewMap(NewCapacity);
for (auto I : Present) {
auto LookupKey = Traits.storageKeyToLookupKey(Buckets[I].first);
- NewMap.set_as_internal(LookupKey, Buckets[I].second, Buckets[I].first);
+ NewMap.set_as_internal(LookupKey, Buckets[I].second, Traits,
+ Buckets[I].first);
}
Buckets.swap(NewMap.Buckets);
Modified: llvm/trunk/include/llvm/DebugInfo/PDB/Native/NamedStreamMap.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo/PDB/Native/NamedStreamMap.h?rev=365974&r1=365973&r2=365974&view=diff
==============================================================================
--- llvm/trunk/include/llvm/DebugInfo/PDB/Native/NamedStreamMap.h (original)
+++ llvm/trunk/include/llvm/DebugInfo/PDB/Native/NamedStreamMap.h Fri Jul 12 16:30:55 2019
@@ -59,7 +59,7 @@ private:
NamedStreamMapTraits HashTraits;
/// Closed hash table from Offset -> StreamNumber, where Offset is the offset
/// of the stream name in NamesBuffer.
- HashTable<support::ulittle32_t, NamedStreamMapTraits> OffsetIndexMap;
+ HashTable<support::ulittle32_t> OffsetIndexMap;
/// Buffer of string data.
std::vector<char> NamesBuffer;
Modified: llvm/trunk/include/llvm/DebugInfo/PDB/Native/PDBFileBuilder.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo/PDB/Native/PDBFileBuilder.h?rev=365974&r1=365973&r2=365974&view=diff
==============================================================================
--- llvm/trunk/include/llvm/DebugInfo/PDB/Native/PDBFileBuilder.h (original)
+++ llvm/trunk/include/llvm/DebugInfo/PDB/Native/PDBFileBuilder.h Fri Jul 12 16:30:55 2019
@@ -97,7 +97,7 @@ private:
PDBStringTableBuilder Strings;
StringTableHashTraits InjectedSourceHashTraits;
- HashTable<SrcHeaderBlockEntry, StringTableHashTraits> InjectedSourceTable;
+ HashTable<SrcHeaderBlockEntry> InjectedSourceTable;
SmallVector<InjectedSourceDescriptor, 2> InjectedSources;
Modified: llvm/trunk/lib/DebugInfo/PDB/Native/NamedStreamMap.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/PDB/Native/NamedStreamMap.cpp?rev=365974&r1=365973&r2=365974&view=diff
==============================================================================
--- llvm/trunk/lib/DebugInfo/PDB/Native/NamedStreamMap.cpp (original)
+++ llvm/trunk/lib/DebugInfo/PDB/Native/NamedStreamMap.cpp Fri Jul 12 16:30:55 2019
@@ -46,8 +46,7 @@ uint32_t NamedStreamMapTraits::lookupKey
return NS->appendStringData(S);
}
-NamedStreamMap::NamedStreamMap()
- : HashTraits(*this), OffsetIndexMap(1, HashTraits) {}
+NamedStreamMap::NamedStreamMap() : HashTraits(*this), OffsetIndexMap(1) {}
Error NamedStreamMap::load(BinaryStreamReader &Stream) {
uint32_t StringBufferSize;
@@ -99,7 +98,7 @@ uint32_t NamedStreamMap::hashString(uint
}
bool NamedStreamMap::get(StringRef Stream, uint32_t &StreamNo) const {
- auto Iter = OffsetIndexMap.find_as(Stream);
+ auto Iter = OffsetIndexMap.find_as(Stream, HashTraits);
if (Iter == OffsetIndexMap.end())
return false;
StreamNo = (*Iter).second;
@@ -123,5 +122,5 @@ uint32_t NamedStreamMap::appendStringDat
}
void NamedStreamMap::set(StringRef Stream, uint32_t StreamNo) {
- OffsetIndexMap.set_as(Stream, support::ulittle32_t(StreamNo));
+ OffsetIndexMap.set_as(Stream, support::ulittle32_t(StreamNo), HashTraits);
}
Modified: llvm/trunk/lib/DebugInfo/PDB/Native/PDBFileBuilder.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/PDB/Native/PDBFileBuilder.cpp?rev=365974&r1=365973&r2=365974&view=diff
==============================================================================
--- llvm/trunk/lib/DebugInfo/PDB/Native/PDBFileBuilder.cpp (original)
+++ llvm/trunk/lib/DebugInfo/PDB/Native/PDBFileBuilder.cpp Fri Jul 12 16:30:55 2019
@@ -34,7 +34,7 @@ using namespace llvm::support;
PDBFileBuilder::PDBFileBuilder(BumpPtrAllocator &Allocator)
: Allocator(Allocator), InjectedSourceHashTraits(Strings),
- InjectedSourceTable(2, InjectedSourceHashTraits) {}
+ InjectedSourceTable(2) {}
PDBFileBuilder::~PDBFileBuilder() {}
@@ -189,7 +189,8 @@ Error PDBFileBuilder::finalizeMsfLayout(
static_cast<uint32_t>(PdbRaw_SrcHeaderBlockVer::SrcVerOne);
Entry.CRC = CRC.getCRC();
StringRef VName = getStringTableBuilder().getStringForId(IS.VNameIndex);
- InjectedSourceTable.set_as(VName, std::move(Entry));
+ InjectedSourceTable.set_as(VName, std::move(Entry),
+ InjectedSourceHashTraits);
}
uint32_t SrcHeaderBlockSize =
Modified: llvm/trunk/unittests/DebugInfo/PDB/HashTableTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/DebugInfo/PDB/HashTableTest.cpp?rev=365974&r1=365973&r2=365974&view=diff
==============================================================================
--- llvm/trunk/unittests/DebugInfo/PDB/HashTableTest.cpp (original)
+++ llvm/trunk/unittests/DebugInfo/PDB/HashTableTest.cpp Fri Jul 12 16:30:55 2019
@@ -27,27 +27,35 @@ using namespace llvm::support;
namespace {
-class HashTableInternals : public HashTable<uint32_t> {
+struct IdentityHashTraits {
+ uint32_t hashLookupKey(uint32_t N) const { return N; }
+ uint32_t storageKeyToLookupKey(uint32_t N) const { return N; }
+ uint32_t lookupKeyToStorageKey(uint32_t N) { return N; }
+};
+
+template <class T = uint32_t>
+class HashTableInternals : public HashTable<T> {
public:
- using HashTable::Buckets;
- using HashTable::Present;
- using HashTable::Deleted;
+ using HashTable<T>::Buckets;
+ using HashTable<T>::Present;
+ using HashTable<T>::Deleted;
};
}
TEST(HashTableTest, TestSimple) {
- HashTableInternals Table;
+ HashTableInternals<> Table;
EXPECT_EQ(0u, Table.size());
EXPECT_GT(Table.capacity(), 0u);
- Table.set_as(3u, 7);
+ IdentityHashTraits Traits;
+ Table.set_as(3u, 7, Traits);
EXPECT_EQ(1u, Table.size());
- ASSERT_NE(Table.end(), Table.find_as(3u));
- EXPECT_EQ(7u, Table.get(3u));
+ ASSERT_NE(Table.end(), Table.find_as(3u, Traits));
+ EXPECT_EQ(7u, Table.get(3u, Traits));
}
TEST(HashTableTest, TestCollision) {
- HashTableInternals Table;
+ HashTableInternals<> Table;
EXPECT_EQ(0u, Table.size());
EXPECT_GT(Table.capacity(), 0u);
@@ -57,33 +65,35 @@ TEST(HashTableTest, TestCollision) {
uint32_t N1 = Table.capacity() + 1;
uint32_t N2 = 2 * N1;
- Table.set_as(N1, 7);
- Table.set_as(N2, 12);
+ IdentityHashTraits Traits;
+ Table.set_as(N1, 7, Traits);
+ Table.set_as(N2, 12, Traits);
EXPECT_EQ(2u, Table.size());
- ASSERT_NE(Table.end(), Table.find_as(N1));
- ASSERT_NE(Table.end(), Table.find_as(N2));
+ ASSERT_NE(Table.end(), Table.find_as(N1, Traits));
+ ASSERT_NE(Table.end(), Table.find_as(N2, Traits));
- EXPECT_EQ(7u, Table.get(N1));
- EXPECT_EQ(12u, Table.get(N2));
+ EXPECT_EQ(7u, Table.get(N1, Traits));
+ EXPECT_EQ(12u, Table.get(N2, Traits));
}
TEST(HashTableTest, TestRemove) {
- HashTableInternals Table;
+ HashTableInternals<> Table;
EXPECT_EQ(0u, Table.size());
EXPECT_GT(Table.capacity(), 0u);
- Table.set_as(1u, 2);
- Table.set_as(3u, 4);
+ IdentityHashTraits Traits;
+ Table.set_as(1u, 2, Traits);
+ Table.set_as(3u, 4, Traits);
EXPECT_EQ(2u, Table.size());
- ASSERT_NE(Table.end(), Table.find_as(1u));
- ASSERT_NE(Table.end(), Table.find_as(3u));
+ ASSERT_NE(Table.end(), Table.find_as(1u, Traits));
+ ASSERT_NE(Table.end(), Table.find_as(3u, Traits));
- EXPECT_EQ(2u, Table.get(1u));
- EXPECT_EQ(4u, Table.get(3u));
+ EXPECT_EQ(2u, Table.get(1u, Traits));
+ EXPECT_EQ(4u, Table.get(3u, Traits));
}
TEST(HashTableTest, TestCollisionAfterMultipleProbes) {
- HashTableInternals Table;
+ HashTableInternals<> Table;
EXPECT_EQ(0u, Table.size());
EXPECT_GT(Table.capacity(), 0u);
@@ -94,17 +104,18 @@ TEST(HashTableTest, TestCollisionAfterMu
uint32_t N2 = N1 + 1;
uint32_t N3 = 2 * N1;
- Table.set_as(N1, 7);
- Table.set_as(N2, 11);
- Table.set_as(N3, 13);
+ IdentityHashTraits Traits;
+ Table.set_as(N1, 7, Traits);
+ Table.set_as(N2, 11, Traits);
+ Table.set_as(N3, 13, Traits);
EXPECT_EQ(3u, Table.size());
- ASSERT_NE(Table.end(), Table.find_as(N1));
- ASSERT_NE(Table.end(), Table.find_as(N2));
- ASSERT_NE(Table.end(), Table.find_as(N3));
-
- EXPECT_EQ(7u, Table.get(N1));
- EXPECT_EQ(11u, Table.get(N2));
- EXPECT_EQ(13u, Table.get(N3));
+ ASSERT_NE(Table.end(), Table.find_as(N1, Traits));
+ ASSERT_NE(Table.end(), Table.find_as(N2, Traits));
+ ASSERT_NE(Table.end(), Table.find_as(N3, Traits));
+
+ EXPECT_EQ(7u, Table.get(N1, Traits));
+ EXPECT_EQ(11u, Table.get(N2, Traits));
+ EXPECT_EQ(13u, Table.get(N3, Traits));
}
TEST(HashTableTest, Grow) {
@@ -112,24 +123,26 @@ TEST(HashTableTest, Grow) {
// guaranteed to trigger a grow. Then verify that the size is the same, the
// capacity is larger, and all the original items are still in the table.
- HashTableInternals Table;
+ HashTableInternals<> Table;
+ IdentityHashTraits Traits;
uint32_t OldCapacity = Table.capacity();
for (uint32_t I = 0; I < OldCapacity; ++I) {
- Table.set_as(OldCapacity + I * 2 + 1, I * 2 + 3);
+ Table.set_as(OldCapacity + I * 2 + 1, I * 2 + 3, Traits);
}
EXPECT_EQ(OldCapacity, Table.size());
EXPECT_GT(Table.capacity(), OldCapacity);
for (uint32_t I = 0; I < OldCapacity; ++I) {
- ASSERT_NE(Table.end(), Table.find_as(OldCapacity + I * 2 + 1));
- EXPECT_EQ(I * 2 + 3, Table.get(OldCapacity + I * 2 + 1));
+ ASSERT_NE(Table.end(), Table.find_as(OldCapacity + I * 2 + 1, Traits));
+ EXPECT_EQ(I * 2 + 3, Table.get(OldCapacity + I * 2 + 1, Traits));
}
}
TEST(HashTableTest, Serialization) {
- HashTableInternals Table;
+ HashTableInternals<> Table;
+ IdentityHashTraits Traits;
uint32_t Cap = Table.capacity();
for (uint32_t I = 0; I < Cap; ++I) {
- Table.set_as(Cap + I * 2 + 1, I * 2 + 3);
+ Table.set_as(Cap + I * 2 + 1, I * 2 + 3, Traits);
}
std::vector<uint8_t> Buffer(Table.calculateSerializedLength());
@@ -139,7 +152,7 @@ TEST(HashTableTest, Serialization) {
// We should have written precisely the number of bytes we calculated earlier.
EXPECT_EQ(Buffer.size(), Writer.getOffset());
- HashTableInternals Table2;
+ HashTableInternals<> Table2;
BinaryStreamReader Reader(Stream);
EXPECT_THAT_ERROR(Table2.load(Reader), Succeeded());
// We should have read precisely the number of bytes we calculated earlier.
@@ -192,20 +205,19 @@ TEST(HashTableTest, NamedStreamMap) {
} while (std::next_permutation(Streams.begin(), Streams.end()));
}
-namespace {
struct FooBar {
uint32_t X;
uint32_t Y;
-};
-} // namespace
+ bool operator==(const FooBar &RHS) const {
+ return X == RHS.X && Y == RHS.Y;
+ }
+};
-namespace llvm {
-namespace pdb {
-template <> struct PdbHashTraits<FooBar> {
+struct FooBarHashTraits {
std::vector<char> Buffer;
- PdbHashTraits() { Buffer.push_back(0); }
+ FooBarHashTraits() { Buffer.push_back(0); }
uint32_t hashLookupKey(StringRef S) const {
return llvm::pdb::hashStringV1(S);
@@ -225,17 +237,16 @@ template <> struct PdbHashTraits<FooBar>
return N;
}
};
-} // namespace pdb
-} // namespace llvm
TEST(HashTableTest, NonTrivialValueType) {
- HashTable<FooBar> Table;
+ HashTableInternals<FooBar> Table;
+ FooBarHashTraits Traits;
uint32_t Cap = Table.capacity();
for (uint32_t I = 0; I < Cap; ++I) {
FooBar F;
F.X = I;
F.Y = I + 1;
- Table.set_as(utostr(I), F);
+ Table.set_as(utostr(I), F, Traits);
}
std::vector<uint8_t> Buffer(Table.calculateSerializedLength());
@@ -245,7 +256,7 @@ TEST(HashTableTest, NonTrivialValueType)
// We should have written precisely the number of bytes we calculated earlier.
EXPECT_EQ(Buffer.size(), Writer.getOffset());
- HashTable<FooBar> Table2;
+ HashTableInternals<FooBar> Table2;
BinaryStreamReader Reader(Stream);
EXPECT_THAT_ERROR(Table2.load(Reader), Succeeded());
// We should have read precisely the number of bytes we calculated earlier.
@@ -253,7 +264,7 @@ TEST(HashTableTest, NonTrivialValueType)
EXPECT_EQ(Table.size(), Table2.size());
EXPECT_EQ(Table.capacity(), Table2.capacity());
- // EXPECT_EQ(Table.Buckets, Table2.Buckets);
- // EXPECT_EQ(Table.Present, Table2.Present);
- // EXPECT_EQ(Table.Deleted, Table2.Deleted);
+ EXPECT_EQ(Table.Buckets, Table2.Buckets);
+ EXPECT_EQ(Table.Present, Table2.Present);
+ EXPECT_EQ(Table.Deleted, Table2.Deleted);
}
More information about the llvm-commits
mailing list