Added per tier pool class rolling average latency #96
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @guptask and @igchor)
cachelib/common/RollingStats.h line 1 at r1 (raw file):
/*
"Also, have you measured how much performance impact this latency tracking have? Could you please add an option to config to enable/disable gathering this latency info?
Ans: I'm working on a compile type flag option.
"
Will you add this option to this PR?
cachelib/common/RollingStats.h line 35 at r1 (raw file):
avg_ *= ratio; ++cnt_; avg_ += value / cnt_;
"
What happens if cnt_ overflows (division by 0?)
Ans: I checked. Even when cnt_ equals 0, the algorithm won't cause divide by 0 error.
"
How's that? I see there's value / cnt_.
cachelib/common/RollingStats.h line 40 at r1 (raw file):
// Return the rolling average. uint64_t estimate() { return static_cast<uint64_t>(avg_);
I think it would be better to return double here. I know you need uint64_t later but this class might also be used for different purposes where double might be more appropriate
cachelib/common/RollingStats.h line 71 at r1 (raw file):
} RollingLatencyTracker& operator=(RollingLatencyTracker&& rhs) noexcept {
didn't you say those are unncessary? (i.e. can't we delete move ctor and assignment operator?)
|
Previously, igchor (Igor Chorążewicz) wrote…
I'm going to measure the performance impact first. If the impact is negligible, I'm in favor of not making it a compile time option. I'm waiting for my machine to be free; it is under use for background eviction data collection. |
|
Previously, igchor (Igor Chorążewicz) wrote…
ok. I'll update it to double |
|
Previously, igchor (Igor Chorążewicz) wrote…
I mentioned that about RollingStats. I have taken those out. I kept the assignment operator for RollingLatencyTracker based on comparison to the LatencyTracker (used by PercentileStats). I don't see it being used currently. |
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @igchor)
cachelib/common/RollingStats.h line 35 at r1 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
"
What happens if cnt_ overflows (division by 0?)
Ans: I checked. Even when cnt_ equals 0, the algorithm won't cause divide by 0 error.
"How's that? I see there's
value / cnt_.
cnt_ is getting incremented before this line. cnt_ is currently uint64_t. Only scenario is can be 0 is if the value rolls around from 2^64-1.
guptask
force-pushed
the
rolling_stats
branch
2 times, most recently
from
6fa3770 to
12e187d
Compare
|
Command used : The data on the right is with |
Please refer to the results shared in a comment below. Based on these results, I think the performance footprint of rolling stats is quite lot. |
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @guptask and @igchor)
cachelib/cachebench/cache/CacheStats.h line 162 at r3 (raw file):
out << folly::sformat( "tid{:2} pid{:2} cid{:4} {:8.2f}{} memorySize:{:8.2f}{} " "free:{:4.2f}% latency:{:8.2f}ns",
Please extend the description - what latency is being reported?
cachelib/common/RollingStats.h line 1 at r1 (raw file):
Previously, guptask (Sounak Gupta) wrote…
Please refer to the results shared in a comment below. Based on these results, I think the performance footprint of rolling stats is quite lot.
Can you run it a few times and average the results? The difference is small enough to be just a run-to-run variance
cachelib/common/RollingStats.h line 35 at r1 (raw file):
Previously, guptask (Sounak Gupta) wrote…
cnt_ is getting in
Can you elaborate? I don;t understand "cnt_ is getting in".
Even if you increase cnt_ you can cause it overflow (when it's equal to std::numeric_limits<uint64_t>::max()) to 0.
guptask
force-pushed
the
rolling_stats
branch
4 times, most recently
from
a834f16 to
b3ec830
Compare
|
Previously, igchor (Igor Chorążewicz) wrote…
changed it to rollingAvgLatency |
|
Previously, igchor (Igor Chorążewicz) wrote…
I ran it 3 times. The difference is small enough to be considered a runtime variance. |
|
Previously, igchor (Igor Chorążewicz) wrote…
Sorry I accidentally pressed send before finishing the sentence. I added the full sentence in another comment which is not showing up in the Reviewable mode. I wrote: You're talking about the same thing. So I added a clause that stops calculating rolling latency once cnt_ reaches numerical limit. |
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @guptask and @igchor)
cachelib/cachebench/cache/CacheStats.h line 162 at r3 (raw file):
Previously, guptask (Sounak Gupta) wrote…
changed it to rollingAvgLatency
I meant "latency of what"? You should specify that it's find latency (unless it is already displayed somewhere?)
cachelib/common/RollingStats.h line 35 at r1 (raw file):
Previously, guptask (Sounak Gupta) wrote…
Sorry I accidentally pressed send before finishing the sentence. I added the full sentence in another comment which is not showing up in the Reviewable mode. I wrote:
"cnt_ is getting incremented before this line. cnt_ is currently uint64_t. Only scenario is can be 0 is if the value rolls around from 2^64-1."You're talking about the same thing. So I added a clause that stops calculating rolling latency once cnt_ reaches numerical limit.
I guess you could just start from 0 in that case. Instead of stopping, just set cnt_ to 0 and move on. You don't need this idel variable in that case as well.
|
Previously, igchor (Igor Chorążewicz) wrote…
Done. |
|
Previously, igchor (Igor Chorążewicz) wrote…
done |
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @igchor)
Summary: fix comment typo Pull Request resolved: facebook#96 Reviewed By: jiayuebao Differential Revision: D33021002 Pulled By: haowu14 fbshipit-source-id: d01887b92e167c9f59af49d027d2377f4740ea26
This change is