-
Notifications
You must be signed in to change notification settings - Fork 124
[L0 v2] Add latency tracker #1892
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
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.
I wrote something like this before when I was investigating latency spikes in enqueue, but I took a different approach. I made a structure that then formed a hierarchy of trackers, and on destruction of the top-most object (per-thread global) printed a (a hacky one-off) histogram of collected data.
Something like:
static per_thread FooTracker foo; // this also had some options to e.g., only collect data if the top-most operation took more than N ns.
void foo() {
TRACKER(&foo); //this took function name and line #
{
TRACKER_SCOPE(&foo, "op1");
// some expensive operation
}
{
TRACKER_SCOPE(&foo); // or anonymous scope
}
}
with the tracker global object usable across module boundaries to track latency of operations in a deep stack of things.
I never cleaned up that code (I can try to dig it somewhere out of my hundreds of UR branches :P), and the histogram implementation was awful (I had to tune buckets by hand), but it gave me a better overall picture of the latency of an operation, rather than just an average.
|
|
||
| private: | ||
| const char *name; | ||
| double avg_{0}; |
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 is this a double and not just an integer value?
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 would still need to cast it to double for calculations in trackValue I think. I'm not sure if it matters really. But I have changed the estimate() return value to be uint64_t
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 would still need to cast it to double for calculations in trackValue I think. I'm not sure if it matters really.
Well, I just prefer not using floating point math if we don't have to, especially for something so simple as sum += value; ++cnt; avg = sum / cnt;. But yeah, I don't think matters all that much.
so that v2::context will be appropriately destroyed
0a537da to
d2f8523
Compare
That makes sense. I think it would be good to have some histogram as well in the future. In CacheLib we used one implemented on top of folly but that's a huge dependency + I'm not sure what's the overhead for that. For this rolling latency the overhead is really not noticeable for things we are measuring. |
Yea, I'd rather we find some good small compact histogram library. Otherwise, we can always put this behind a ifdef in cmake.
Yeah, let's go with this simple implementation for now. |
Please take a look at: #1912 |
The latency tracker is useful for measuring and optimizing specific parts of code that are hard to profile using external tracers.
Sample output: