Do not compensate for rounding error when calculating tier sizes #43
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 8, 2022
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @igchor)
Compensation results in ratios being different than originially specified.
vinser52
pushed a commit
to vinser52/CacheLib
that referenced
this pull request
Feb 10, 2022
…m#43) Compensation results in ratios being different than originially specified.
guptask
pushed a commit
to guptask/CacheLib
that referenced
this pull request
Aug 11, 2022
…m#43) Compensation results in ratios being different than originially specified.
vinser52
pushed a commit
that referenced
this pull request
Sep 13, 2022
Compensation results in ratios being different than originially specified.
byrnedj
pushed a commit
to byrnedj/CacheLib
that referenced
this pull request
Nov 12, 2022
…m#43) Compensation results in ratios being different than originially specified.
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
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>
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.
Compensation results in ratios being different than originially specified.
This change is