Skip to content

Commit f507394

Browse files
igchorigchor
committed
Add combined locking support for MMContainer
through withEvictionIterator function. Also, expose config option to enable and disable combined locking. withEvictionIterator is implemented as en extra function, getEvictionIterator() is still there and it's behavior hasn't changed.
1 parent 227cac0 commit f507394

File tree

16 files changed

+229
-86
lines changed

16 files changed

+229
-86
lines changed

‎cachelib/allocator/CacheAllocator.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1660,7 +1660,7 @@ class CacheAllocator : public CacheBase {
16601660
// @return An evicted item or nullptr if there is no suitable candidate.
16611661
Item* findEviction(PoolId pid, ClassId cid);
16621662

1663-
using EvictionIterator = typename MMContainer::Iterator;
1663+
using EvictionIterator = typename MMContainer::LockedIterator;
16641664

16651665
// Advance the current iterator and try to evict a regular item
16661666
//

‎cachelib/allocator/MM2Q-inl.h

Lines changed: 13 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -241,29 +241,17 @@ bool MM2Q::Container<T, HookPtr>::add(T& node) noexcept {
241241
}
242242

243243
template <typename T, MM2Q::Hook<T> T::*HookPtr>
244-
typename MM2Q::Container<T, HookPtr>::Iterator
244+
typename MM2Q::Container<T, HookPtr>::LockedIterator
245245
MM2Q::Container<T, HookPtr>::getEvictionIterator() const noexcept {
246-
// we cannot use combined critical sections with folly::DistributedMutex here
247-
// because the lock is held for the lifetime of the eviction iterator. In
248-
// other words, the abstraction of the iterator just does not lend itself well
249-
// to combinable critical sections as the user can hold the lock for an
250-
// arbitrary amount of time outside a lambda-friendly piece of code (eg. they
251-
// can return the iterator from functions, pass it to functions, etc)
252-
//
253-
// it would be theoretically possible to refactor this interface into
254-
// something like the following to allow combining
255-
//
256-
// mm2q.withEvictionIterator([&](auto iterator) {
257-
// // user code
258-
// });
259-
//
260-
// at the time of writing it is unclear if the gains from combining are
261-
// reasonable justification for the codemod required to achieve combinability
262-
// as we don't expect this critical section to be the hotspot in user code.
263-
// This is however subject to change at some time in the future as and when
264-
// this assertion becomes false.
265246
LockHolder l(*lruMutex_);
266-
return Iterator{std::move(l), lru_.rbegin()};
247+
return LockedIterator{std::move(l), lru_.rbegin()};
248+
}
249+
250+
template <typename T, MM2Q::Hook<T> T::*HookPtr>
251+
template <typename F>
252+
void MM2Q::Container<T, HookPtr>::withEvictionIterator(F&& fun) {
253+
LockHolder l(*lruMutex_);
254+
fun(Iterator{lru_.rbegin()});
267255
}
268256

269257
template <typename T, MM2Q::Hook<T> T::*HookPtr>
@@ -462,8 +450,9 @@ void MM2Q::Container<T, HookPtr>::reconfigureLocked(const Time& currTime) {
462450

463451
// Iterator Context Implementation
464452
template <typename T, MM2Q::Hook<T> T::*HookPtr>
465-
MM2Q::Container<T, HookPtr>::Iterator::Iterator(
466-
LockHolder l, const typename LruList::Iterator& iter) noexcept
467-
: LruList::Iterator(iter), l_(std::move(l)) {}
453+
MM2Q::Container<T, HookPtr>::LockedIterator::LockedIterator(
454+
LockHolder l, const Iterator& iter) noexcept
455+
: Iterator(iter), l_(std::move(l)) {}
456+
468457
} // namespace cachelib
469458
} // namespace facebook

‎cachelib/allocator/MM2Q.h

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ class MM2Q {
6868
enum LruType { Warm, WarmTail, Hot, Cold, ColdTail, NumTypes };
6969

7070
// Config class for MM2Q
71+
// TODO: implement support for useCombinedLockForIterators
7172
struct Config {
7273
// Create from serialized config
7374
explicit Config(SerializationConfigType configState)
@@ -347,22 +348,24 @@ class MM2Q {
347348
Container(const Container&) = delete;
348349
Container& operator=(const Container&) = delete;
349350

351+
using Iterator = typename LruList::Iterator;
352+
350353
// context for iterating the MM container. At any given point of time,
351354
// there can be only one iterator active since we need to lock the LRU for
352355
// iteration. we can support multiple iterators at same time, by using a
353356
// shared ptr in the context for the lock holder in the future.
354-
class Iterator : public LruList::Iterator {
357+
class LockedIterator : public Iterator {
355358
public:
356359
// noncopyable but movable.
357-
Iterator(const Iterator&) = delete;
358-
Iterator& operator=(const Iterator&) = delete;
360+
LockedIterator(const LockedIterator&) = delete;
361+
LockedIterator& operator=(const LockedIterator&) = delete;
359362

360-
Iterator(Iterator&&) noexcept = default;
363+
LockedIterator(LockedIterator&&) noexcept = default;
361364

362365
// 1. Invalidate this iterator
363366
// 2. Unlock
364367
void destroy() {
365-
LruList::Iterator::reset();
368+
Iterator::reset();
366369
if (l_.owns_lock()) {
367370
l_.unlock();
368371
}
@@ -373,15 +376,15 @@ class MM2Q {
373376
if (!l_.owns_lock()) {
374377
l_.lock();
375378
}
376-
LruList::Iterator::resetToBegin();
379+
Iterator::resetToBegin();
377380
}
378381

379382
private:
380383
// private because it's easy to misuse and cause deadlock for MM2Q
381-
Iterator& operator=(Iterator&&) noexcept = default;
384+
LockedIterator& operator=(LockedIterator&&) noexcept = default;
382385

383386
// create an lru iterator with the lock being held.
384-
Iterator(LockHolder l, const typename LruList::Iterator& iter) noexcept;
387+
LockedIterator(LockHolder l, const Iterator& iter) noexcept;
385388

386389
// only the container can create iterators
387390
friend Container<T, HookPtr>;
@@ -422,7 +425,7 @@ class MM2Q {
422425

423426
// same as the above but uses an iterator context. The iterator is updated
424427
// on removal of the corresponding node to point to the next node. The
425-
// iterator context holds the lock on the lru.
428+
// iterator context is responsible for locking.
426429
//
427430
// iterator will be advanced to the next node after removing the node
428431
//
@@ -445,7 +448,12 @@ class MM2Q {
445448
// Obtain an iterator that start from the tail and can be used
446449
// to search for evictions. This iterator holds a lock to this
447450
// container and only one such iterator can exist at a time
448-
Iterator getEvictionIterator() const noexcept;
451+
LockedIterator getEvictionIterator() const noexcept;
452+
453+
// Execute provided function under container lock. Function gets
454+
// Iterator passed as parameter.
455+
template <typename F>
456+
void withEvictionIterator(F&& f);
449457

450458
// get the current config as a copy
451459
Config getConfig() const;

‎cachelib/allocator/MMLru-inl.h

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -212,10 +212,21 @@ bool MMLru::Container<T, HookPtr>::add(T& node) noexcept {
212212
}
213213

214214
template <typename T, MMLru::Hook<T> T::*HookPtr>
215-
typename MMLru::Container<T, HookPtr>::Iterator
215+
typename MMLru::Container<T, HookPtr>::LockedIterator
216216
MMLru::Container<T, HookPtr>::getEvictionIterator() const noexcept {
217217
LockHolder l(*lruMutex_);
218-
return Iterator{std::move(l), lru_.rbegin()};
218+
return LockedIterator{std::move(l), lru_.rbegin()};
219+
}
220+
221+
template <typename T, MMLru::Hook<T> T::*HookPtr>
222+
template <typename F>
223+
void MMLru::Container<T, HookPtr>::withEvictionIterator(F&& fun) {
224+
if (config_.useCombinedLockForIterators) {
225+
lruMutex_->lock_combine([this, &fun]() { fun(Iterator{lru_.rbegin()}); });
226+
} else {
227+
LockHolder lck{*lruMutex_};
228+
fun(Iterator{lru_.rbegin()});
229+
}
219230
}
220231

221232
template <typename T, MMLru::Hook<T> T::*HookPtr>
@@ -360,8 +371,9 @@ void MMLru::Container<T, HookPtr>::reconfigureLocked(const Time& currTime) {
360371

361372
// Iterator Context Implementation
362373
template <typename T, MMLru::Hook<T> T::*HookPtr>
363-
MMLru::Container<T, HookPtr>::Iterator::Iterator(
364-
LockHolder l, const typename LruList::Iterator& iter) noexcept
365-
: LruList::Iterator(iter), l_(std::move(l)) {}
374+
MMLru::Container<T, HookPtr>::LockedIterator::LockedIterator(
375+
LockHolder l, const Iterator& iter) noexcept
376+
: Iterator(iter), l_(std::move(l)) {}
377+
366378
} // namespace cachelib
367379
} // namespace facebook

‎cachelib/allocator/MMLru.h

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -144,15 +144,17 @@ class MMLru {
144144
bool updateOnR,
145145
bool tryLockU,
146146
uint8_t ipSpec,
147-
uint32_t mmReconfigureInterval)
147+
uint32_t mmReconfigureInterval,
148+
bool useCombinedLockForIterators = false)
148149
: defaultLruRefreshTime(time),
149150
lruRefreshRatio(ratio),
150151
updateOnWrite(updateOnW),
151152
updateOnRead(updateOnR),
152153
tryLockUpdate(tryLockU),
153154
lruInsertionPointSpec(ipSpec),
154155
mmReconfigureIntervalSecs(
155-
std::chrono::seconds(mmReconfigureInterval)) {}
156+
std::chrono::seconds(mmReconfigureInterval)),
157+
useCombinedLockForIterators(useCombinedLockForIterators) {}
156158

157159
Config() = default;
158160
Config(const Config& rhs) = default;
@@ -195,6 +197,9 @@ class MMLru {
195197
// lruInsertionPointSpec = 2, we insert at a point 1/4th from tail
196198
uint8_t lruInsertionPointSpec{0};
197199

200+
// Whether to use combined locking for withEvictionIterator.
201+
bool useCombinedLockForIterators{false};
202+
198203
// Minimum interval between reconfigurations. If 0, reconfigure is never
199204
// called.
200205
std::chrono::seconds mmReconfigureIntervalSecs{};
@@ -234,22 +239,24 @@ class MMLru {
234239
Container(const Container&) = delete;
235240
Container& operator=(const Container&) = delete;
236241

242+
using Iterator = typename LruList::Iterator;
243+
237244
// context for iterating the MM container. At any given point of time,
238245
// there can be only one iterator active since we need to lock the LRU for
239246
// iteration. we can support multiple iterators at same time, by using a
240247
// shared ptr in the context for the lock holder in the future.
241-
class Iterator : public LruList::Iterator {
248+
class LockedIterator : public Iterator {
242249
public:
243250
// noncopyable but movable.
244-
Iterator(const Iterator&) = delete;
245-
Iterator& operator=(const Iterator&) = delete;
251+
LockedIterator(const LockedIterator&) = delete;
252+
LockedIterator& operator=(const LockedIterator&) = delete;
246253

247-
Iterator(Iterator&&) noexcept = default;
254+
LockedIterator(LockedIterator&&) noexcept = default;
248255

249256
// 1. Invalidate this iterator
250257
// 2. Unlock
251258
void destroy() {
252-
LruList::Iterator::reset();
259+
Iterator::reset();
253260
if (l_.owns_lock()) {
254261
l_.unlock();
255262
}
@@ -260,15 +267,15 @@ class MMLru {
260267
if (!l_.owns_lock()) {
261268
l_.lock();
262269
}
263-
LruList::Iterator::resetToBegin();
270+
Iterator::resetToBegin();
264271
}
265272

266273
private:
267274
// private because it's easy to misuse and cause deadlock for MMLru
268-
Iterator& operator=(Iterator&&) noexcept = default;
275+
LockedIterator& operator=(LockedIterator&&) noexcept = default;
269276

270277
// create an lru iterator with the lock being held.
271-
Iterator(LockHolder l, const typename LruList::Iterator& iter) noexcept;
278+
LockedIterator(LockHolder l, const Iterator& iter) noexcept;
272279

273280
// only the container can create iterators
274281
friend Container<T, HookPtr>;
@@ -307,10 +314,9 @@ class MMLru {
307314
// state of node is unchanged.
308315
bool remove(T& node) noexcept;
309316

310-
using Iterator = Iterator;
311317
// same as the above but uses an iterator context. The iterator is updated
312318
// on removal of the corresponding node to point to the next node. The
313-
// iterator context holds the lock on the lru.
319+
// iterator context is responsible for locking.
314320
//
315321
// iterator will be advanced to the next node after removing the node
316322
//
@@ -330,7 +336,12 @@ class MMLru {
330336
// Obtain an iterator that start from the tail and can be used
331337
// to search for evictions. This iterator holds a lock to this
332338
// container and only one such iterator can exist at a time
333-
Iterator getEvictionIterator() const noexcept;
339+
LockedIterator getEvictionIterator() const noexcept;
340+
341+
// Execute provided function under container lock. Function gets
342+
// iterator passed as parameter.
343+
template <typename F>
344+
void withEvictionIterator(F&& f);
334345

335346
// get copy of current config
336347
Config getConfig() const;

‎cachelib/allocator/MMTinyLFU-inl.h

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -214,10 +214,17 @@ bool MMTinyLFU::Container<T, HookPtr>::add(T& node) noexcept {
214214
}
215215

216216
template <typename T, MMTinyLFU::Hook<T> T::*HookPtr>
217-
typename MMTinyLFU::Container<T, HookPtr>::Iterator
217+
typename MMTinyLFU::Container<T, HookPtr>::LockedIterator
218218
MMTinyLFU::Container<T, HookPtr>::getEvictionIterator() const noexcept {
219219
LockHolder l(lruMutex_);
220-
return Iterator{std::move(l), *this};
220+
return LockedIterator{std::move(l), *this};
221+
}
222+
223+
template <typename T, MMTinyLFU::Hook<T> T::*HookPtr>
224+
template <typename F>
225+
void MMTinyLFU::Container<T, HookPtr>::withEvictionIterator(F&& fun) {
226+
LockHolder l(lruMutex_);
227+
fun(Iterator{*this});
221228
}
222229

223230
template <typename T, MMTinyLFU::Hook<T> T::*HookPtr>
@@ -347,13 +354,18 @@ void MMTinyLFU::Container<T, HookPtr>::reconfigureLocked(const Time& currTime) {
347354
lruRefreshTime_.store(lruRefreshTime, std::memory_order_relaxed);
348355
}
349356

357+
// Locked Iterator Context Implementation
358+
template <typename T, MMTinyLFU::Hook<T> T::*HookPtr>
359+
MMTinyLFU::Container<T, HookPtr>::LockedIterator::LockedIterator(
360+
LockHolder l, const Container<T, HookPtr>& c) noexcept
361+
: Iterator(c), l_(std::move(l)) {}
362+
350363
// Iterator Context Implementation
351364
template <typename T, MMTinyLFU::Hook<T> T::*HookPtr>
352365
MMTinyLFU::Container<T, HookPtr>::Iterator::Iterator(
353-
LockHolder l, const Container<T, HookPtr>& c) noexcept
366+
const Container<T, HookPtr>& c) noexcept
354367
: c_(c),
355368
tIter_(c.lru_.getList(LruType::Tiny).rbegin()),
356-
mIter_(c.lru_.getList(LruType::Main).rbegin()),
357-
l_(std::move(l)) {}
369+
mIter_(c.lru_.getList(LruType::Main).rbegin()) {}
358370
} // namespace cachelib
359371
} // namespace facebook

0 commit comments

Comments
 (0)