Skip to content

Conversation

guptask
Copy link

@guptask guptask commented Jul 21, 2022

No description provided.

@igchor
Copy link
Owner

igchor commented Jul 21, 2022

@guptask can you make this PR against pmem/CacheLib develop branch? I think it does not depend on my branch.

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?

@igchor
Copy link
Owner

igchor commented Jul 21, 2022

  1. Why does RollingStats::estimate() return uint64_t and not double?
  2. What happens if cnt_ overflows (division by 0?)
  3. Do we need to implement: RollingLatencyTracker& operator=(RollingLatencyTracker&& rhs) and RollingLatencyTracker(RollingLatencyTracker&& rhs) at all? Can't we just delete them?

@guptask
Copy link
Author

guptask commented Jul 21, 2022

  1. Why does RollingStats::estimate() return uint64_t and not double?
    Ans: Currently latency is reported as uint64_t by PercentileStats::Estimates class. So even though I track rolling average as double, I typecast it to unit64_t in the get call.
  1. 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.
  1. Do we need to implement: RollingLatencyTracker& operator=(RollingLatencyTracker&& rhs) and RollingLatencyTracker(RollingLatencyTracker&& rhs) at all? Can't we just delete them?
    Ans: I took these out.

@guptask
Copy link
Author

guptask commented Jul 21, 2022

@guptask can you make this PR against pmem/CacheLib develop branch? I think it does not depend on my branch.

Ans: I don't think pmem/CacheLib develop branch has the required changes to support my commit. My changes is based on getAllocationClassStats() transferring stats to cachebench. That change is part of your branch. Besides Bogi is developing her dynamic thresholding based eviction policy against your branch. She needs the rolling average allocation latency. This PR does not need to be merged here. It can either be transferred to Bogi's branch or be re-raised against develop once your in_progress branch gets merged with develop.

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.

@guptask guptask closed this Jul 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants