facebook/sapling
1
1
Fork 0
mirror of https://github.com/facebook/sapling.git synced 2024-10-07 07:17:55 +03:00
Code Issues Projects Releases Wiki Activity

only evict when interest handle is dropped if blob wasn't reloaded

Browse Source 
Summary:
There was a bug in BlobCache where, if you had an interest handle to a
blob, but that blob was evicted anyway and then something else caused
it to be reloaded, dropping your interest handle would cause the blob
to be incorrectly evicted since the reference counts were no longer
compatible. Add a version to cache items and only decrement the
reference count on an item if the interest handle and item agree.

Reviewed By: strager

Differential Revision: D13405144

fbshipit-source-id: aee052bf777e7225551c3ae2b8b69a99f4f77691
This commit is contained in:
Chad Austin 2018-12-10 19:28:14 -08:00 committed by Facebook Github Bot
parent f0003e3735
commit fe557ace7c
5 changed files with 111 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
eden/fs/model/Hash.h
View File

@ -33,7 +33,7 @@ class Hash : boost::totally_ordered<Hash> {
/**
* Create a 0-initialized hash
*/
constexpr Hash() : bytes_{} {}
constexpr Hash() noexcept : bytes_{} {}
explicit constexpr Hash(const Storage& bytes) : bytes_{bytes} {}

68
eden/fs/store/BlobCache.cpp
View File

@ -11,24 +11,24 @@
#include <folly/MapUtil.h>
#include <folly/logging/xlog.h>
#include "eden/fs/model/Blob.h"
#include "eden/fs/utils/IDGen.h"
namespace facebook {
namespace eden {
BlobInterestHandle::BlobInterestHandle(std::weak_ptr<const Blob> blob)
: blob_{std::move(blob)} {
// No need to initialize hash_ because blobCache_ is unset.
}
BlobInterestHandle::BlobInterestHandle(
std::weak_ptr<BlobCache> blobCache,
const Hash& hash,
std::weak_ptr<const Blob> blob)
: blobCache_{std::move(blobCache)}, hash_{hash}, blob_{std::move(blob)} {}
std::weak_ptr<const Blob> blob,
uint64_t generation) noexcept
: blobCache_{std::move(blobCache)},
hash_{hash},
blob_{std::move(blob)},
cacheItemGeneration_{generation} {}
void BlobInterestHandle::reset() noexcept {
if (auto blobCache = blobCache_.lock()) {
blobCache->dropInterestHandle(hash_);
blobCache->dropInterestHandle(hash_, cacheItemGeneration_);
}
blobCache_.reset();
}
@ -66,6 +66,8 @@ BlobCache::BlobCache(size_t maximumCacheSizeBytes, size_t minimumEntryCount)
BlobCache::~BlobCache() {}
BlobCache::GetResult BlobCache::get(const Hash& hash, Interest interest) {
XLOG(DBG6) << "BlobCache::get " << hash;
// Acquires BlobCache's lock upon destruction by calling dropInterestHandle,
// so ensure that, if an exception is thrown below, the ~BlobInterestHandle
// runs after the lock is released.
@ -75,6 +77,7 @@ BlobCache::GetResult BlobCache::get(const Hash& hash, Interest interest) {
auto* item = folly::get_ptr(state->items, hash);
if (!item) {
XLOG(DBG6) << "BlobCache::get missed";
++state->missCount;
return GetResult{};
}
@ -84,7 +87,8 @@ BlobCache::GetResult BlobCache::get(const Hash& hash, Interest interest) {
interestHandle.blob_ = item->blob;
break;
case Interest::WantHandle:
interestHandle = BlobInterestHandle{shared_from_this(), hash, item->blob};
interestHandle = BlobInterestHandle{
shared_from_this(), hash, item->blob, item->generation};
++item->referenceCount;
break;
case Interest::LikelyNeededAgain:
@ -100,6 +104,8 @@ BlobCache::GetResult BlobCache::get(const Hash& hash, Interest interest) {
break;
}
XLOG(DBG6) << "BlobCache::get hit";
// TODO: Should we avoid promoting if interest is UnlikelyNeededAgain?
// For now, we'll try not to be too clever.
state->evictionQueue.splice(
@ -111,6 +117,8 @@ BlobCache::GetResult BlobCache::get(const Hash& hash, Interest interest) {
BlobInterestHandle BlobCache::insert(
std::shared_ptr<const Blob> blob,
Interest interest) {
XLOG(DBG6) << "BlobCache::insert " << blob->getHash();
// Acquires BlobCache's lock upon destruction by calling dropInterestHandle,
// so ensure that, if an exception is thrown below, the ~BlobInterestHandle
// runs after the lock is released.
@ -119,15 +127,21 @@ BlobInterestHandle BlobCache::insert(
auto hash = blob->getHash();
auto size = blob->getSize();
auto cacheItemGeneration = generateUniqueID();
if (interest == Interest::WantHandle) {
// This can throw, so do it before inserting into items.
interestHandle = BlobInterestHandle{shared_from_this(), hash, blob};
interestHandle =
BlobInterestHandle{shared_from_this(), hash, blob, cacheItemGeneration};
} else {
interestHandle.blob_ = blob;
}
XLOG(DBG6) << " creating entry with generation=" << cacheItemGeneration;
auto state = state_.wlock();
auto [iter, inserted] = state->items.try_emplace(hash, std::move(blob));
auto [iter, inserted] =
state->items.try_emplace(hash, std::move(blob), cacheItemGeneration);
// noexcept from here until `try`
switch (interest) {
case Interest::UnlikelyNeededAgain:
@ -137,8 +151,8 @@ BlobInterestHandle BlobCache::insert(
++iter->second.referenceCount;
break;
}
auto* itemPtr = &iter->second;
if (inserted) {
auto* itemPtr = &iter->second;
try {
state->evictionQueue.push_back(itemPtr);
} catch (std::exception&) {
@ -149,8 +163,11 @@ BlobInterestHandle BlobCache::insert(
state->totalSize += size;
evictUntilFits(*state);
} else {
XLOG(DBG6) << " duplicate entry, using generation " << itemPtr->generation;
// Inserting duplicate entry - use its generation.
interestHandle.cacheItemGeneration_ = itemPtr->generation;
state->evictionQueue.splice(
state->evictionQueue.end(), state->evictionQueue, iter->second.index);
state->evictionQueue.end(), state->evictionQueue, itemPtr->index);
}
return interestHandle;
}
@ -160,6 +177,14 @@ bool BlobCache::contains(const Hash& hash) const {
return 1 == state->items.count(hash);
}
void BlobCache::clear() {
XLOG(DBG6) << "BlobCache::clear";
auto state = state_.wlock();
state->totalSize = 0;
state->items.clear();
state->evictionQueue.clear();
}
BlobCache::Stats BlobCache::getStats() const {
auto state = state_.rlock();
Stats stats;
@ -172,7 +197,10 @@ BlobCache::Stats BlobCache::getStats() const {
return stats;
}
void BlobCache::dropInterestHandle(const Hash& hash) noexcept {
void BlobCache::dropInterestHandle(
const Hash& hash,
uint64_t generation) noexcept {
XLOG(DBG6) << "dropInterestHandle " << hash << " generation=" << generation;
auto state = state_.wlock();
auto* item = folly::get_ptr(state->items, hash);
@ -181,6 +209,12 @@ void BlobCache::dropInterestHandle(const Hash& hash) noexcept {
return;
}
if (generation != item->generation) {
// Item was evicted and re-added between creating and dropping the interest
// handle.
return;
}
if (item->referenceCount == 0) {
XLOG(WARN)
<< "Reference count on item for " << hash
@ -196,6 +230,10 @@ void BlobCache::dropInterestHandle(const Hash& hash) noexcept {
}
void BlobCache::evictUntilFits(State& state) noexcept {
XLOG(DBG6) << "state.totalSize=" << state.totalSize
<< ", maximumCacheSizeBytes_=" << maximumCacheSizeBytes_
<< ", evictionQueue.size()=" << state.evictionQueue.size()
<< ", minimumEntryCount_=" << minimumEntryCount_;
while (state.totalSize > maximumCacheSizeBytes_ &&
state.evictionQueue.size() > minimumEntryCount_) {
evictOne(state);
@ -210,6 +248,8 @@ void BlobCache::evictOne(State& state) noexcept {
}
void BlobCache::evictItem(State& state, CacheItem* item) noexcept {
XLOG(DBG6) << "evicting " << item->blob->getHash()
<< " generation=" << item->generation;
auto size = item->blob->getSize();
// TODO: Releasing this BlobPtr here can run arbitrary deleters which could,
// in theory, try to reacquire the BlobCache's lock. The blob could be

43
eden/fs/store/BlobCache.h
View File

@ -27,26 +27,14 @@ class BlobCache;
*/
class BlobInterestHandle {
public:
BlobInterestHandle() = default;
BlobInterestHandle() noexcept = default;
~BlobInterestHandle() {
~BlobInterestHandle() noexcept {
reset();
}
BlobInterestHandle(BlobInterestHandle&& other) noexcept
: blobCache_{std::move(other.blobCache_)}, hash_{other.hash_} {
// We don't need to clear other.hash_ because it's only referenced when
// blobCache_ is not expired.
}
BlobInterestHandle& operator=(BlobInterestHandle&& other) noexcept {
if (this != &other) {
reset();
blobCache_ = std::move(other.blobCache_);
hash_ = other.hash_;
}
return *this;
}
BlobInterestHandle(BlobInterestHandle&& other) noexcept = default;
BlobInterestHandle& operator=(BlobInterestHandle&& other) noexcept = default;
/**
* If this is a valid interest handle, and the blob is still in cache, return
@ -59,13 +47,14 @@ class BlobInterestHandle {
void reset() noexcept;
private:
explicit BlobInterestHandle(std::weak_ptr<const Blob> blob);
BlobInterestHandle(
std::weak_ptr<BlobCache> blobCache,
const Hash& hash,
std::weak_ptr<const Blob> blob);
std::weak_ptr<const Blob> blob,
uint64_t generation) noexcept;
std::weak_ptr<BlobCache> blobCache_;
// hash_ is only accessed if blobCache_ is non-expired.
Hash hash_;
@ -73,6 +62,10 @@ class BlobInterestHandle {
// retrieving it anyway.
std::weak_ptr<const Blob> blob_;
// Only causes eviction if this matches the corresponding
// CacheItem::generation.
uint64_t cacheItemGeneration_{0};
friend class BlobCache;
};
@ -171,6 +164,11 @@ class BlobCache : public std::enable_shared_from_this<BlobCache> {
*/
bool contains(const Hash& hash) const;
/**
* Evicts everything from cache.
*/
void clear();
/**
* Return information about the current size of the cache and the total number
* of hits and misses.
@ -193,7 +191,8 @@ class BlobCache : public std::enable_shared_from_this<BlobCache> {
// WARNING: leaves index unset. Since the items map and evictionQueue are
// circular, initialization of index must happen after the CacheItem is
// constructed.
explicit CacheItem(BlobPtr b) : blob{std::move(b)} {}
explicit CacheItem(BlobPtr b, uint64_t g)
: blob{std::move(b)}, generation{g} {}
BlobPtr blob;
std::list<CacheItem*>::iterator index;
@ -201,6 +200,10 @@ class BlobCache : public std::enable_shared_from_this<BlobCache> {
/// Incremented on every LikelyNeededAgain or WantInterestHandle.
/// Decremented on every dropInterestHandle. Evicted if it reaches zero.
uint64_t referenceCount{0};
/// Given a unique value upon allocation. Used to verify InterestHandle
// matches this specific item.
uint64_t generation{0};
};
struct State {
@ -216,7 +219,7 @@ class BlobCache : public std::enable_shared_from_this<BlobCache> {
uint64_t dropCount{0};
};
void dropInterestHandle(const Hash& hash) noexcept;
void dropInterestHandle(const Hash& hash, uint64_t generation) noexcept;
explicit BlobCache(size_t maximumCacheSizeBytes, size_t minimumEntryCount);
void evictUntilFits(State& state) noexcept;

30
eden/fs/store/test/BlobCacheTest.cpp
View File

@ -199,6 +199,13 @@ TEST(BlobCache, redundant_inserts_are_ignored) {
EXPECT_EQ(9, cache->getStats().totalSizeInBytes);
}
TEST(BlobCache, redundant_insert_does_not_invalidate_interest_handles) {
auto cache = BlobCache::create(10, 0);
auto handle3 = cache->insert(blob3, BlobCache::Interest::WantHandle);
cache->insert(blob3, BlobCache::Interest::WantHandle);
EXPECT_TRUE(handle3.getBlob());
}
TEST(
BlobCache,
fetching_blob_from_interest_handle_moves_to_back_of_eviction_queue) {
@ -230,3 +237,26 @@ TEST(BlobCache, interest_handle_can_return_blob_even_if_it_was_evicted) {
EXPECT_EQ(blob4, handle4.getBlob());
EXPECT_EQ(blob5, handle5.getBlob());
}
TEST(
BlobCache,
dropping_interest_handle_does_not_evict_if_item_has_been_reloaded_after_clear) {
auto cache = BlobCache::create(10, 0);
auto handle3 = cache->insert(blob3, BlobCache::Interest::WantHandle);
cache->clear();
cache->insert(blob3);
handle3.reset();
EXPECT_TRUE(cache->contains(hash3));
}
TEST(
BlobCache,
dropping_interest_handle_does_not_evict_if_item_has_been_reloaded_after_eviction) {
auto cache = BlobCache::create(10, 0);
auto handle3 = cache->insert(blob3, BlobCache::Interest::WantHandle);
cache->insert(blob4);
cache->insert(blob5);
auto handle3again = cache->insert(blob3, BlobCache::Interest::WantHandle);
handle3.reset();
EXPECT_TRUE(cache->contains(hash3));
}

5
eden/win/fs/edenfs.vcxproj
View File

@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<?xml version="1.0" encoding="utf-8"?>
<Project DefaultTargets="Build" ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<ItemGroup Label="ProjectConfigurations">
<ProjectConfiguration Include="Debug|Win32">
@ -199,6 +199,7 @@
<ClCompile Include="..\..\fs\store\StoreResult.cpp" />
<ClCompile Include="..\..\fs\tracing\Tracing.cpp" />
<ClCompile Include="..\..\fs\utils\Clock.cpp" />
<ClCompile Include="..\..\fs\utils\IDGen.cpp" />
<ClCompile Include="..\..\fs\utils\PathFuncs.cpp" />
<ClCompile Include="..\..\fs\utils\TimeUtil.cpp" />
<ClCompile Include="..\..\fs\utils\UnboundedQueueExecutor.cpp" />
@ -332,4 +333,4 @@
<ImportGroup Label="ExtensionTargets">
<Import Project="D:\edenwin64\vcpkg\scripts\buildsystems\msbuild\vcpkg.targets" Condition="Exists('D:\edenwin64\vcpkg\scripts\buildsystems\msbuild\vcpkg.targets')" />
</ImportGroup>
</Project>
</Project>