Skip to content

[Upstreaming] Added per pool per class used size metrics #70

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 2 commits into from

Conversation

guptask
Copy link
Collaborator

@guptask guptask commented May 10, 2022

This per pool size statistics reports the current size of each memory pool. Existing implementation does not report this statistic accurately. `allocAttempts - allocFailed - regularItemsEvictions - chainedItemsEvictions won't report this value accurately because pool size grows in multiples of block sizes. This statistics will be reported in the final report generated by cachebench.


This change is Reviewable

@guptask guptask self-assigned this May 10, 2022
@guptask guptask changed the title Added per pool per class used size metrics [Upstreaming] Added per pool per class used size metrics May 10, 2022
@guptask guptask force-pushed the per_tier_stats branch 3 times, most recently from 1fee456 to 4090a0b Compare May 11, 2022 03:28
Copy link

@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 10 files reviewed, 1 unresolved discussion (waiting on @guptask)


cachelib/allocator/tests/AllocatorHitStatsTest.h line 398 at r1 (raw file):

    ASSERT_EQ(globalCacheStats.poolUsedSize.size(),
              MemoryPoolManager::kMaxPools);
    ASSERT_EQ(4521459712, globalCacheStats.poolUsedSize[0]);

please calculate this value based on slab size

@guptask guptask force-pushed the per_tier_stats branch 7 times, most recently from 6134eec to d908f8a Compare May 17, 2022 20:52
@guptask guptask force-pushed the per_tier_stats branch 2 times, most recently from 1c7c5d7 to 242e304 Compare May 19, 2022 08:55
@guptask guptask requested a review from igchor May 19, 2022 16:20
Copy link

@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.

:lgtm:

Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on @guptask)


cachelib/allocator/CacheAllocator.h line 1119 at r2 (raw file):

  bool isOnShm() const noexcept { return isOnShm_; }

  ClassId getAllocClassId(PoolId pid,

I'm not sure if Meta will be OK with adding such functions. Can't we use some existing ones? Some of the tests are also marked as friends of this class, so maybe we could use that?

@guptask
Copy link
Collaborator Author

guptask commented May 24, 2022

cachelib/allocator/CacheAllocator.h line 1119 at r2 (raw file):

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

I'm not sure if Meta will be OK with adding such functions. Can't we use some existing ones? Some of the tests are also marked as friends of this class, so maybe we could use that?

We can raise the PR as it is here and see what Meta has to say about it. I think having an API approach is cleaner.

@guptask
Copy link
Collaborator Author

guptask commented May 24, 2022

cachelib/allocator/tests/AllocatorHitStatsTest.h line 398 at r1 (raw file):

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

please calculate this value based on slab size

I didn't understand your point. This check was pre-existing.

@guptask guptask closed this May 25, 2022
igchor pushed a commit to igchor/CacheLib-1 that referenced this pull request Mar 24, 2023
byrnedj added a commit to byrnedj/CacheLib that referenced this pull request Jul 24, 2023
byrnedj added a commit to byrnedj/CacheLib that referenced this pull request Mar 12, 2024
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