-
Notifications
You must be signed in to change notification settings - Fork 6.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cache simulator: Add a ghost cache for admission control and a hybrid row-block cache. #5534
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HaoyuHuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
26cd6a4
to
c3a11ed
Compare
@HaoyuHuang has updated the pull request. Re-import the pull request |
@HaoyuHuang has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HaoyuHuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a pass and left a few comments.
@@ -50,24 +181,48 @@ BlockCacheTraceSimulator::BlockCacheTraceSimulator( | |||
cache_configurations_(cache_configurations) {} | |||
|
|||
Status BlockCacheTraceSimulator::InitializeCaches() { | |||
const std::string ghost_cache_prefix = "ghost_"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth renaming to kGhostCachePrefix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I renamed it.
uint64_t accesses = hits + misses; | ||
return static_cast<double>(misses * 100.0 / accesses); | ||
void PrioritizedCacheSimulator::Access(const BlockCacheTraceRecord& access) { | ||
bool is_cache_miss; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIt: initialize to avoid linter warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initialized.
user_accesses_ = 0; | ||
user_misses_ = 0; | ||
} | ||
double miss_ratio() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added const for all functions that do not update member variables.
} | ||
return static_cast<double>(num_misses_ * 100.0 / num_accesses_); | ||
} | ||
uint64_t total_accesses() { return num_accesses_; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added const for all functions that do not update member variables.
} | ||
uint64_t total_accesses() { return num_accesses_; } | ||
|
||
double user_miss_ratio() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added const for all functions that do not update member variables.
} | ||
return static_cast<double>(user_misses_ * 100.0 / user_accesses_); | ||
} | ||
uint64_t user_accesses() { return user_accesses_; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added const for all functions that do not update member variables.
bool is_user_access, bool* is_cache_miss, bool* admitted, | ||
bool update_metrics); | ||
|
||
Cache::Priority ComputeBlockPriority(const BlockCacheTraceRecord& access); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added const for all functions that do not update member variables.
const uint64_t kGhostCacheSize = 1024 * 1024; | ||
} // namespace | ||
|
||
class CacheSimulatorTest : public DBTestBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the base class does not have to be DBTestBase
since many of its database operations, e.g. Open
, Close
, Put
, Flush
, etc. are not needed. Maybe consider use testing::Test
or testing::WithParamInterface
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I changed to testing::Test.
@HaoyuHuang has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few comments.
@@ -1321,7 +1321,7 @@ backupable_db_test: utilities/backupable/backupable_db_test.o $(LIBOBJECTS) $(TE | |||
checkpoint_test: utilities/checkpoint/checkpoint_test.o $(LIBOBJECTS) $(TESTHARNESS) | |||
$(AM_LINK) | |||
|
|||
cache_simulator_test: utilities/simulator_cache/cache_simulator_test.o db/db_test_util.o $(LIBOBJECTS) $(TESTHARNESS) | |||
cache_simulator_test: utilities/simulator_cache/cache_simulator_test.o $(LIBOBJECTS) $(TESTHARNESS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. This can easily be forgotten.
}; | ||
|
||
// A map stores get_id to a map of row keys. For each row key, it stores a | ||
// pair of booleans. The first bool is true when we observe a miss upon the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's not a pair of booleans. It's a boolean and an enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the comment.
sim_cache_->Insert(access.block_key, /*value=*/nullptr, access.block_size, | ||
/*deleter=*/nullptr, /*handle=*/nullptr); | ||
if (handle != nullptr) { | ||
sim_cache_->Release(handle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remind me of why release the handle upon a hit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is that each entry maintains a ref counter. It is incremented by one inside the lookup so we need to call Release to decrement the ref count.
@HaoyuHuang has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HaoyuHuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@HaoyuHuang merged this pull request in 1a59b6e. |
… row-block cache. (facebook#5534) Summary: This PR adds a ghost cache for admission control. Specifically, it admits an entry on its second access. It also adds a hybrid row-block cache that caches the referenced key-value pairs of a Get/MultiGet request instead of its blocks. Pull Request resolved: facebook#5534 Test Plan: make clean && COMPILE_WITH_ASAN=1 make check -j32 Differential Revision: D16101124 Pulled By: HaoyuHuang fbshipit-source-id: b99edda6418a888e94eb40f71ece45d375e234b1
This PR adds a ghost cache for admission control. Specifically, it admits an entry on its second access.
It also adds a hybrid row-block cache that caches the referenced key-value pairs of a Get/MultiGet request instead of its blocks.
Test plan: make clean && COMPILE_WITH_ASAN=1 make check -j32