Skip to content

[upstream] added per tier pool class rolling average latency #45

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

Closed
wants to merge 1 commit into from

Conversation

guptask
Copy link

@guptask guptask commented Dec 20, 2022

This change is Reviewable

@guptask guptask self-assigned this Dec 20, 2022
@guptask guptask force-pushed the upstream_rolling_stats branch from 38b17a8 to caa1318 Compare December 20, 2022 22:55
Copy link
Member

@igchor igchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @guptask)


cachelib/allocator/Cache.h line 89 at r1 (raw file):

  // TODO: come up with some reasonable number
  static constexpr unsigned kMaxTiers = 2;

Can we not include any references to tiers in this PR? We could add support for tracking latency per class, per pool, right?


cachelib/allocator/CacheAllocator-inl.h line 2231 at r1 (raw file):

template <typename CacheTrait>
double CacheAllocator<CacheTrait>::slabsApproxFreePercentage(TierId tid) const {

I think this should not part of the latency tracking patch.


cachelib/allocator/CacheAllocator-inl.h line 2236 at r1 (raw file):

template <typename CacheTrait>
AllocationClassBaseStat CacheAllocator<CacheTrait>::getAllocationClassStats(

Same here, you should probably just store allocLatenencyNs.

You can rebase onto #42 perhaps and just use getACStats which I've added if you think it will be easier.

@guptask
Copy link
Author

guptask commented Dec 28, 2022

cachelib/allocator/CacheAllocator-inl.h line 2231 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

I think this should not part of the latency tracking patch.

Done.

@guptask
Copy link
Author

guptask commented Dec 28, 2022

cachelib/allocator/CacheAllocator-inl.h line 2236 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Same here, you should probably just store allocLatenencyNs.

You can rebase onto #42 perhaps and just use getACStats which I've added if you think it will be easier.

Done.

@guptask
Copy link
Author

guptask commented Dec 28, 2022

cachelib/allocator/Cache.h line 89 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Can we not include any references to tiers in this PR? We could add support for tracking latency per class, per pool, right?

Done.

@guptask
Copy link
Author

guptask commented Dec 28, 2022

I'm closing this PR and raising a new one (#47) that is rebased against Igor's AC Stats upstream PR (#42)

@guptask guptask closed this Dec 28, 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