Fixed total cache size in CacheMemoryStats #38
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vinser52
approved these changes
Feb 7, 2022
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @victoria-mcgrath)
igchor
approved these changes
Feb 7, 2022
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @victoria-mcgrath)
This comment was marked as resolved.
This comment was marked as resolved.
victoria-mcgrath
force-pushed
the
cache_size_stats
branch
from
9c9faee to
c879923
Compare
victoria-mcgrath
commented
Feb 8, 2022
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status:
vinser52
pushed a commit
to vinser52/CacheLib
that referenced
this pull request
Feb 10, 2022
Return a sum of sizes of each tier instead of just 1st tier's size.
guptask
pushed a commit
to guptask/CacheLib
that referenced
this pull request
Aug 11, 2022
Return a sum of sizes of each tier instead of just 1st tier's size.
vinser52
pushed a commit
that referenced
this pull request
Sep 13, 2022
Return a sum of sizes of each tier instead of just 1st tier's size.
byrnedj
pushed a commit
to byrnedj/CacheLib
that referenced
this pull request
Nov 12, 2022
Return a sum of sizes of each tier instead of just 1st tier's size.
byrnedj
added a commit
to byrnedj/CacheLib
that referenced
this pull request
Dec 12, 2022
- Use markExclusive only for eviction (findEviction and evictForSlabRelease) but not for item movement. moveForSlabRelease relies on markMoving(). Only allow to mark item as exclusive if ref count is 0. This ensures that after item is marked eviction cannot fail. This makes it possible to return NULL handle immediately from find if item is marked as exclusive. markMoving() does have those restrictions and still allows readers to obtain a handle to a moving item. Also, add option to use combined locking for MMContainer iteration. - Block readers when item is moving This simplifies moving code a lot. We don't need to worry about any failure other than allocation failure since item is protected from any readers (and by extension, removes). Co-authored-by: Igor Chorążewicz <igor.chorazewicz@intel.com>
byrnedj
pushed a commit
to byrnedj/CacheLib
that referenced
this pull request
Dec 15, 2022
author Chorazewicz, Igor <igor.chorazewicz@intel.com> 1632834667 +0200 committer Daniel Byrne <byrnedj12@gmail.com> 1671070203 -0800 Initial multi-tier support implementation Extend CompressedPtr to work with multiple tiers Now it's size is 8 bytes intead of 4. Original CompressedPtr stored only some offset with a memory Allocator. For multi-tier implementation, this is not enough. We must also store tierId and when uncompressing, select a proper allocator. An alternative could be to just resign from CompressedPtr but they are leveraged to allow the cache to be mapped to different addresses on shared memory. Changing CompressedPtr impacted CacheItem size - it increased from 32 to 44 bytes. Implemented async Item movement between tiers (with corrected behaivour of tryEvictToNextMemoryTier). Fix ReaperSkippingSlabTraversalWhileSlabReleasing test The issue was caused by incorrect behaviour of the CacheAllocator<CacheTrait>::tryEvictToNextMemoryTier method in case the evicted item is expired. We cannot simply return a handle to it, but we need to remove it from the access container and MM container. Enable workarounds in tests Add basic multi-tier test Set correct size for each memory tier Do not compensate for rounding error when calculating tier sizes (pmem#43) Compensation results in ratios being different than originially specified. Fixed total cache size in CacheMemoryStats (pmem#38) Return a sum of sizes of each tier instead of just 1st tier's size. Issue75 rebased (pmem#88) * pmem#75: Use actual tier sizes (rounded down to slab size and decreased by header size) when creating new memory pools * Added getPoolSize method to calculate combined pool size for all tiers; added pool size validation to tests * Explicitly specified type for totalCacheSize to avoid overflow * Minor test change * Reworked tests * Minor change * Deleted redundant tests * Deleted unused constant * First set of changes to cache configuration API to enable multi-tier caches (facebook#138) Summary: These changes introduce per-tier cache configuration required to implement features discussed here: facebook#102. These specific changes enable single DRAM tier configs only which are compatible with the current version of cachelib. Configuration API will be expanded as multi-tier changes in other parts of the library are introduced. Pull Request resolved: facebook#138 Reviewed By: therealgymmy Differential Revision: D36189766 Pulled By: jiayuebao fbshipit-source-id: 947aa0cd800ea6accffc1b7b6b0c9693aa7fc0a5 Co-authored-by: Victoria McGrath <victoria.mcgrath@intel.com> Fix eviction flow and removeCb calls Without this fix removeCb called even in case when Item is moved between tiers. Fix issue with "Destorying an unresolved handle" The issue happened when ReadHandleImpl ctor needs to destroy waitContext_ because addWaitContextForMovingItem() returns false. So before destroying waitContext_ we are calling discard method to notify ~ItemWaitContext() that Item is ready. Fix slab release code Get tier id of item before calling any function on allocator (which needs the tierID).
byrnedj
added a commit
to byrnedj/CacheLib
that referenced
this pull request
Dec 15, 2022
- Use markExclusive only for eviction (findEviction and evictForSlabRelease) but not for item movement. moveForSlabRelease relies on markMoving(). Only allow to mark item as exclusive if ref count is 0. This ensures that after item is marked eviction cannot fail. This makes it possible to return NULL handle immediately from find if item is marked as exclusive. markMoving() does have those restrictions and still allows readers to obtain a handle to a moving item. Also, add option to use combined locking for MMContainer iteration. - Block readers when item is moving This simplifies moving code a lot. We don't need to worry about any failure other than allocation failure since item is protected from any readers (and by extension, removes). Co-authored-by: Igor Chorążewicz <igor.chorazewicz@intel.com>
byrnedj
pushed a commit
to byrnedj/CacheLib
that referenced
this pull request
Dec 22, 2022
* New class MemoryTierCacheConfig allows to configure a memory tier. Setting tier size and location of a file for file-backed memory are supported in this initial implementation; * New member, vector of memory tiers, is added to class CacheAllocatorConfig. * New test suite, chelib/allocator/tests/MemoryTiersTest.cpp, demonstrates the usage of and tests extended config API. Integrate Memory Tier config API with CacheAllocator. Enabled memory tier config API for cachebench. Assumes NUMA bindings support - this commit was done before tiering support enabled. fix memBind call replaces Add MemoryTierCacheConfig::fromShm() (commit 1c7c15d) Move validation code for memory tiers to validate() method and convert ratios to sizes lazily Do not compensate for rounding error when calculating tier sizes (pmem#43) Compensation results in ratios being different than originially specified. Fixed total cache size in CacheMemoryStats (pmem#38) Return a sum of sizes of each tier instead of just 1st tier's size. Issue75 rebased (pmem#88) * pmem#75: Use actual tier sizes (rounded down to slab size and decreased by header size) when creating new memory pools * Added getPoolSize method to calculate combined pool size for all tiers; added pool size validation to tests * Explicitly specified type for totalCacheSize to avoid overflow * Minor test change * Reworked tests * Minor change * Deleted redundant tests * Deleted unused constant * First set of changes to cache configuration API to enable multi-tier caches (facebook#138) Summary: These changes introduce per-tier cache configuration required to implement features discussed here: facebook#102. These specific changes enable single DRAM tier configs only which are compatible with the current version of cachelib. Configuration API will be expanded as multi-tier changes in other parts of the library are introduced. Pull Request resolved: facebook#138 Reviewed By: therealgymmy Differential Revision: D36189766 Pulled By: jiayuebao fbshipit-source-id: 947aa0cd800ea6accffc1b7b6b0c9693aa7fc0a5 Co-authored-by: Victoria McGrath <victoria.mcgrath@intel.com>
byrnedj
pushed a commit
to byrnedj/CacheLib
that referenced
this pull request
Jan 3, 2023
* New class MemoryTierCacheConfig allows to configure a memory tier. Setting tier size and location of a file for file-backed memory are supported in this initial implementation; * New member, vector of memory tiers, is added to class CacheAllocatorConfig. * New test suite, chelib/allocator/tests/MemoryTiersTest.cpp, demonstrates the usage of and tests extended config API. Integrate Memory Tier config API with CacheAllocator. Enabled memory tier config API for cachebench. Assumes NUMA bindings support - this commit was done before tiering support enabled. fix memBind call replaces Add MemoryTierCacheConfig::fromShm() (commit 1c7c15d) Move validation code for memory tiers to validate() method and convert ratios to sizes lazily Do not compensate for rounding error when calculating tier sizes (pmem#43) Compensation results in ratios being different than originially specified. Fixed total cache size in CacheMemoryStats (pmem#38) Return a sum of sizes of each tier instead of just 1st tier's size. Issue75 rebased (pmem#88) * pmem#75: Use actual tier sizes (rounded down to slab size and decreased by header size) when creating new memory pools * Added getPoolSize method to calculate combined pool size for all tiers; added pool size validation to tests * Explicitly specified type for totalCacheSize to avoid overflow * Minor test change * Reworked tests * Minor change * Deleted redundant tests * Deleted unused constant * First set of changes to cache configuration API to enable multi-tier caches (facebook#138) Summary: These changes introduce per-tier cache configuration required to implement features discussed here: facebook#102. These specific changes enable single DRAM tier configs only which are compatible with the current version of cachelib. Configuration API will be expanded as multi-tier changes in other parts of the library are introduced. Pull Request resolved: facebook#138 Reviewed By: therealgymmy Differential Revision: D36189766 Pulled By: jiayuebao fbshipit-source-id: 947aa0cd800ea6accffc1b7b6b0c9693aa7fc0a5 Co-authored-by: Victoria McGrath <victoria.mcgrath@intel.com>
byrnedj
added a commit
to byrnedj/CacheLib
that referenced
this pull request
Feb 2, 2023
Based on cs part 2 upstream patch. - Use markForEviction only for eviction (findEviction and evictForSlabRelease) but not for item movement. moveForSlabRelease relies on markMoving(). Only allow to mark item as exclusive if ref count is 0. This ensures that after item is marked eviction cannot fail. This makes it possible to return NULL handle immediately from find if item is marked as exclusive. markMoving() does have those restrictions and still allows readers to obtain a handle to a moving item. Also, add option to use combined locking for MMContainer iteration. - Block readers when item is moving This simplifies moving code a lot. We don't need to worry about any failure other than allocation failure since item is protected from any readers (and by extension, removes). Co-authored-by: Igor Chorążewicz <igor.chorazewicz@intel.com>
byrnedj
added a commit
to byrnedj/CacheLib
that referenced
this pull request
Feb 10, 2023
- transparent item movement - multi-tier combined locking with exclusive bit (pmem#38) with refactored incRef to support returning the result of markMoving (fail if already moving or exclusvie bit is set) option. - add tests (updated for numa bindings - post combined locking) for transparent item movement
byrnedj
added a commit
to byrnedj/CacheLib
that referenced
this pull request
Feb 15, 2023
- transparent item movement - multi-tier combined locking with exclusive bit (pmem#38) with refactored incRef to support returning the result of markMoving (fail if already moving or exclusvie bit is set) option. - add tests (updated for numa bindings - post combined locking) for transparent item movement
byrnedj
added a commit
to byrnedj/CacheLib
that referenced
this pull request
Feb 16, 2023
- transparent item movement - multi-tier combined locking with exclusive bit (pmem#38) with refactored incRef to support returning the result of markMoving (fail if already moving or exclusvie bit is set) option. - add tests (updated for numa bindings - post combined locking) for transparent item movement
byrnedj
added a commit
to byrnedj/CacheLib
that referenced
this pull request
Feb 24, 2023
- transparent item movement - multi-tier combined locking with exclusive bit (pmem#38) with refactored incRef to support returning the result of markMoving (fail if already moving or exclusvie bit is set) option. - add tests (updated for numa bindings - post combined locking) for transparent item movement
byrnedj
added a commit
to byrnedj/CacheLib
that referenced
this pull request
Feb 27, 2023
- transparent item movement - multi-tier combined locking with exclusive bit (pmem#38) with refactored incRef to support returning the result of markMoving (fail if already moving or exclusvie bit is set) option. - add tests (updated for numa bindings - post combined locking) for transparent item movement
byrnedj
added a commit
to byrnedj/CacheLib
that referenced
this pull request
Feb 27, 2023
- transparent item movement - multi-tier combined locking with exclusive bit (pmem#38) with refactored incRef to support returning the result of markMoving (fail if already moving or exclusvie bit is set) option. - add tests (updated for numa bindings - post combined locking) for transparent item movement
byrnedj
added a commit
to byrnedj/CacheLib
that referenced
this pull request
Feb 27, 2023
- transparent item movement - multi-tier combined locking with exclusive bit (pmem#38) with refactored incRef to support returning the result of markMoving (fail if already moving or exclusvie bit is set) option. - add tests (updated for numa bindings - post combined locking) for transparent item movement
byrnedj
added a commit
to byrnedj/CacheLib
that referenced
this pull request
Feb 28, 2023
- transparent item movement - multi-tier combined locking with exclusive bit (pmem#38) with refactored incRef to support returning the result of markMoving (fail if already moving or exclusvie bit is set) option. - add tests (updated for numa bindings - post combined locking) for transparent item movement
byrnedj
added a commit
to byrnedj/CacheLib
that referenced
this pull request
Mar 3, 2023
- transparent item movement - multi-tier combined locking with exclusive bit (pmem#38) with refactored incRef to support returning the result of markMoving (fail if already moving or exclusvie bit is set) option. - add tests (updated for numa bindings - post combined locking) for transparent item movement
byrnedj
added a commit
to byrnedj/CacheLib
that referenced
this pull request
Mar 3, 2023
- transparent item movement - multi-tier combined locking with exclusive bit (pmem#38) with refactored incRef to support returning the result of markMoving (fail if already moving or exclusvie bit is set) option. - add tests (updated for numa bindings - post combined locking) for transparent item movement
byrnedj
added a commit
to byrnedj/CacheLib
that referenced
this pull request
Jul 24, 2023
- transparent item movement - multi-tier combined locking with exclusive bit (pmem#38) with refactored incRef to support returning the result of markMoving (fail if already moving or exclusvie bit is set) option. - add tests (updated for numa bindings - post combined locking) for transparent item movement
byrnedj
added a commit
to byrnedj/CacheLib
that referenced
this pull request
Mar 12, 2024
- transparent item movement - multi-tier combined locking with exclusive bit (pmem#38) with refactored incRef to support returning the result of markMoving (fail if already moving or exclusvie bit is set) option. - add tests (updated for numa bindings - post combined locking) for transparent item movement
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CacheMemoryStats CacheAllocator::getCacheMemoryStats() incorrectly returned cache size of the 0-th memory tier as a total cache size.
This change is