Skip to content

Commit ee74fc6

Browse files
benlangmuircachemeifyoucan
authored andcommitted
[llvm][cas] Remove OnDiskHashMappedTrie's unused HintT
`insertLazy()` was intended to skip some of the subtrie search if given a hint from a previous call to `find()`. But in practice we never passed in such a pointer and this code is unused and untested. Remove it to simplify the implementation. (cherry picked from commit e96683a)
1 parent 301328f commit ee74fc6

File tree

2 files changed

+20
-81
lines changed

2 files changed

+20
-81
lines changed

llvm/include/llvm/CAS/OnDiskHashMappedTrie.h

Lines changed: 14 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -97,31 +97,6 @@ class OnDiskHashMappedTrie {
9797
MutableArrayRef<char> Data;
9898
};
9999

100-
private:
101-
struct HintT {
102-
explicit operator ValueProxy() const {
103-
ValueProxy Value;
104-
Value.Data = MutableArrayRef<char>(
105-
const_cast<char *>(reinterpret_cast<const char *>(P)), I);
106-
Value.Hash = ArrayRef<uint8_t>(nullptr, B);
107-
return Value;
108-
}
109-
110-
explicit HintT(ConstValueProxy Value)
111-
: P(Value.Data.data()), I(Value.Data.size()), B(Value.Hash.size()) {
112-
// Spot-check that this really was a hint.
113-
assert(Value.Data.size() <= UINT16_MAX);
114-
assert(Value.Hash.size() <= UINT16_MAX);
115-
assert(Value.Hash.data() == nullptr);
116-
}
117-
118-
HintT(const void *P, uint16_t I, uint16_t B) : P(P), I(I), B(B) {}
119-
120-
const void *P = nullptr;
121-
uint16_t I = 0;
122-
uint16_t B = 0;
123-
};
124-
125100
public:
126101
template <class ProxyT> class PointerImpl {
127102
public:
@@ -133,50 +108,34 @@ class OnDiskHashMappedTrie {
133108

134109
const ProxyT &operator*() const {
135110
assert(IsValue);
136-
return ValueOrHint;
111+
return Value;
137112
}
138113
const ProxyT *operator->() const {
139114
assert(IsValue);
140-
return &ValueOrHint;
115+
return &Value;
141116
}
142117

143118
PointerImpl() = default;
144119

145120
protected:
146121
PointerImpl(FileOffset Offset, ProxyT Value)
147-
: PointerImpl(Value, Offset, /*IsHint=*/false, /*IsValue=*/true) {}
148-
149-
explicit PointerImpl(FileOffset Offset, HintT H)
150-
: PointerImpl(ValueProxy(H), Offset, /*IsHint=*/true,
151-
/*IsValue=*/false) {}
152-
153-
PointerImpl(ProxyT ValueOrHint, FileOffset Offset, bool IsHint,
154-
bool IsValue)
155-
: ValueOrHint(ValueOrHint), OffsetLow32((uint64_t)Offset.get()),
156-
OffsetHigh16((uint64_t)Offset.get() >> 32), IsHint(IsHint),
157-
IsValue(IsValue) {
158-
checkOffset(Offset);
122+
: PointerImpl(Value, Offset, /*IsValue=*/true) {}
123+
124+
PointerImpl(ProxyT Value, FileOffset Offset, bool IsValue)
125+
: Value(Value), OffsetLow32((uint64_t)Offset.get()),
126+
OffsetHigh16((uint64_t)Offset.get() >> 32), IsValue(IsValue) {
127+
if (IsValue)
128+
checkOffset(Offset);
159129
}
160130

161131
static void checkOffset(FileOffset Offset) {
162132
assert(Offset.get() > 0);
163133
assert((uint64_t)Offset.get() < (1LL << 48));
164134
}
165135

166-
std::optional<HintT> getHint(const OnDiskHashMappedTrie &This) const {
167-
if (!IsHint)
168-
return std::nullopt;
169-
HintT H(ValueOrHint);
170-
assert(H.P == &This && "Expected hint to be for This");
171-
if (H.P != &This)
172-
return std::nullopt;
173-
return H;
174-
}
175-
176-
ProxyT ValueOrHint;
136+
ProxyT Value;
177137
uint32_t OffsetLow32 = 0;
178138
uint16_t OffsetHigh16 = 0;
179-
bool IsHint = false;
180139
bool IsValue = false;
181140
};
182141

@@ -194,7 +153,7 @@ class OnDiskHashMappedTrie {
194153
class pointer : public PointerImpl<ValueProxy> {
195154
public:
196155
operator const_pointer() const {
197-
return const_pointer(ValueOrHint, getOffset(), IsHint, IsValue);
156+
return const_pointer(Value, getOffset(), IsValue);
198157
}
199158

200159
pointer() = default;
@@ -205,8 +164,6 @@ class OnDiskHashMappedTrie {
205164
};
206165

207166
pointer getMutablePointer(const_pointer CP) {
208-
if (std::optional<HintT> H = CP.getHint(*this))
209-
return pointer(CP.getOffset(), *H);
210167
if (!CP)
211168
return pointer();
212169
ValueProxy V{CP->Hash, MutableArrayRef(const_cast<char *>(CP->Data.data()),
@@ -254,25 +211,17 @@ class OnDiskHashMappedTrie {
254211
/// The in-memory \a HashMappedTrie uses LazyAtomicPointer to synchronize
255212
/// simultaneous writes, but that seems dangerous to use in a memory-mapped
256213
/// file in case a process crashes in the busy state.
257-
Expected<pointer> insertLazy(const_pointer Hint, ArrayRef<uint8_t> Hash,
258-
LazyInsertOnConstructCB OnConstruct = nullptr,
259-
LazyInsertOnLeakCB OnLeak = nullptr);
260214
Expected<pointer> insertLazy(ArrayRef<uint8_t> Hash,
261215
LazyInsertOnConstructCB OnConstruct = nullptr,
262-
LazyInsertOnLeakCB OnLeak = nullptr) {
263-
return insertLazy(const_pointer(), Hash, OnConstruct, OnLeak);
264-
}
216+
LazyInsertOnLeakCB OnLeak = nullptr);
265217

266-
Expected<pointer> insert(const_pointer Hint, const ConstValueProxy &Value) {
267-
return insertLazy(Hint, Value.Hash, [&](FileOffset, ValueProxy Allocated) {
218+
Expected<pointer> insert(const ConstValueProxy &Value) {
219+
return insertLazy(Value.Hash, [&](FileOffset, ValueProxy Allocated) {
268220
assert(Allocated.Hash == Value.Hash);
269221
assert(Allocated.Data.size() == Value.Data.size());
270222
llvm::copy(Value.Data, Allocated.Data.begin());
271223
});
272224
}
273-
Expected<pointer> insert(const ConstValueProxy &Value) {
274-
return insert(const_pointer(), Value);
275-
}
276225

277226
size_t size() const;
278227
size_t capacity() const;

llvm/lib/CAS/OnDiskHashMappedTrie.cpp

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -722,16 +722,13 @@ OnDiskHashMappedTrie::find(ArrayRef<uint8_t> Hash) const {
722722
// Try to set the content.
723723
SubtrieSlotValue V = S.load(Index);
724724
if (!V)
725-
return const_pointer(S.getFileOffset(),
726-
HintT(this, Index, *IndexGen.StartBit));
725+
return const_pointer();
727726

728727
// Check for an exact match.
729728
if (V.isData()) {
730729
HashMappedTrieHandle::RecordData D = Trie.getRecord(V);
731-
return D.Proxy.Hash == Hash
732-
? const_pointer(D.getFileOffset(), D.Proxy)
733-
: const_pointer(S.getFileOffset(),
734-
HintT(this, Index, *IndexGen.StartBit));
730+
return D.Proxy.Hash == Hash ? const_pointer(D.getFileOffset(), D.Proxy)
731+
: const_pointer();
735732
}
736733

737734
Index = IndexGen.next();
@@ -750,7 +747,7 @@ void SubtrieHandle::reinitialize(uint32_t StartBit, uint32_t NumBits) {
750747
}
751748

752749
Expected<OnDiskHashMappedTrie::pointer>
753-
OnDiskHashMappedTrie::insertLazy(const_pointer Hint, ArrayRef<uint8_t> Hash,
750+
OnDiskHashMappedTrie::insertLazy(ArrayRef<uint8_t> Hash,
754751
LazyInsertOnConstructCB OnConstruct,
755752
LazyInsertOnLeakCB OnLeak) {
756753
HashMappedTrieHandle Trie = Impl->Trie;
@@ -763,14 +760,7 @@ OnDiskHashMappedTrie::insertLazy(const_pointer Hint, ArrayRef<uint8_t> Hash,
763760
return std::move(Err);
764761

765762
IndexGenerator IndexGen = Trie.getIndexGen(*S, Hash);
766-
767-
size_t Index;
768-
if (std::optional<HintT> H = Hint.getHint(*this)) {
769-
S = SubtrieHandle::getFromFileOffset(Trie.getRegion(), Hint.getOffset());
770-
Index = IndexGen.hint(H->I, H->B);
771-
} else {
772-
Index = IndexGen.next();
773-
}
763+
size_t Index = IndexGen.next();
774764

775765
// FIXME: Add non-assertion based checks for data corruption that would
776766
// otherwise cause infinite loops in release builds, instead calling
@@ -1346,7 +1336,7 @@ OnDiskHashMappedTrie::create(const Twine &PathTwine, const Twine &TrieNameTwine,
13461336
}
13471337

13481338
Expected<OnDiskHashMappedTrie::pointer>
1349-
OnDiskHashMappedTrie::insertLazy(const_pointer Hint, ArrayRef<uint8_t> Hash,
1339+
OnDiskHashMappedTrie::insertLazy(ArrayRef<uint8_t> Hash,
13501340
LazyInsertOnConstructCB OnConstruct,
13511341
LazyInsertOnLeakCB OnLeak) {
13521342
report_fatal_error("not supported");

0 commit comments

Comments
 (0)