-
Notifications
You must be signed in to change notification settings - Fork 525
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
Perf(common): optimize cache promotion and demotion #2949
Perf(common): optimize cache promotion and demotion #2949
Conversation
This is a performance test. #include <benchmark/benchmark.h>
static void GetWithDuplicate(benchmark::State& state){
auto cache = std::make_shared<LRUCache<int, int>>(5, nullptr);
for (int i = 0; i < 5; i++) {
cache->Put(i, i);
}
int v;
for (auto _ : state) {
for (int k = 0; k < 5; k++) {
cache->Get(k, &v);
assert(k == v);
}
}
}
BENCHMARK(GetWithDuplicate);
static void GetWithoutDuplicate(benchmark::State& state){
auto cache = std::make_shared<LRUCache<int, int>>(5, nullptr);
for (int i = 0; i < 5; i++) {
cache->Put(i, i);
}
int v;
for (auto _ : state) {
for (int k = 0; k < 5; k++) {
cache->GetWithSplice(k, &v);
assert(k == v);
}
}
}
BENCHMARK(GetWithoutDuplicate);
BENCHMARK_MAIN(); |
ccbba07
to
5b20ee4
Compare
cicheck |
2 similar comments
cicheck |
cicheck |
5b20ee4
to
e7ab404
Compare
cicheck |
2 similar comments
cicheck |
cicheck |
@Ziy1-Tan Hello, this is my first encounter with CI. In fact, I've added some relevant tests that should completely cover my modifications. But CI tells me coverage ratio 72.9 < expectd 73. I don’t know if this is because my modifications are not up to standard or if it’s due to other modules. I hope I can get your help. :) |
Maybe I can add some tests for |
Nice, try to add some unit tests. Maybe |
Thanks~ |
ll_.push_back(key); | ||
cache_[key] = --ll_.end(); | ||
size_++; | ||
if (cacheMetrics_ != nullptr) { |
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.
Why delete these lines?
@@ -699,16 +696,8 @@ bool SglLRUCache<K, KeyTraits>::MoveBack(const K &key) { | |||
if (iter == cache_.end()) { | |||
return false; | |||
} | |||
// delete the old value | |||
RemoveElement(iter->second); |
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.
This function also include some metrics code, maybe you should consider it?
cicheck |
e7ab404
to
76d8db6
Compare
cicheck |
3 similar comments
cicheck |
cicheck |
cicheck |
76d8db6
to
b6f6f74
Compare
cicheck |
3 similar comments
cicheck |
cicheck |
cicheck |
b6f6f74
to
f105ea1
Compare
cicheck |
1 similar comment
cicheck |
f105ea1
to
f558e70
Compare
cicheck |
1 similar comment
cicheck |
f558e70
to
63ef637
Compare
cicheck |
Signed-off-by: zztaki <[email protected]>
Signed-off-by: zztaki <[email protected]>
63ef637
to
e1f772c
Compare
cicheck |
Since the line coverage and branch coverage of the existing |
close #2982 |
What problem does this PR solve?
Issue Number: #2946
Problem Summary: current cache promotion and demotion need to copy a duplica, erase old elem and reinsert duplica.
What is changed and how it works?
What's Changed: optimize cache promotion and demotion using
list.splice
and add some tests.How it Works:
list.splice
will update cache object priority without object duplica, list reinsertion and hashtable update.Side effects(Breaking backward compatibility? Performance regression?):
Check List