Simplify CacheAllocatorConfig implementation for memory tiers #13
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
It's implementation is mostly based on PosixShmSegment. Also, extend ShmManager and ShmSegmentOpts to support this new segment type.
After introducing file segment type, nameToKey_ does not provide enough information to recover/remove segments on restart. This commit fixes that by replacing nameToKey_ with nameToOpts_. Previously, the Key from nameToKey_ map was only used in a single DCHECK().
* 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.
Run centos and debian workflows on push and PR
Publish changes from innersouce
to allow using new configureMemoryTiers() API with legacy behavior. Move validation code for memory tiers to validate() method and convert ratios to sizes lazily (on get)..
It wrongly assumed that the only possible segment type is PosixSysV segment.
Config api integration
Run tests on CI
FBSW-208 Moved common segment code for posix and file shm segments into ShmCommon
Run long tests (navy/bench) every day on CI
|
Honestly, I like this version better because it seems cleaner and safer. I do agree that dealing with cache-global settings is the problem here. What could we do instead of propagating every global setting to every tier? Would it make more sense to create a kind of property container for cache-wide settings which tiers could reference? |
igchor
force-pushed
the
config_api_simplify
branch
from
24f0bc2 to
a2834af
Compare
igchor
commented
Nov 23, 2021
There was a problem hiding this comment.
Right, this might be one possible approach - but I'm not sure if we should do this now. If you like this solution then I'm OK with merging it as soon as tests will pass.
Reviewable status: 0 of 2 files reviewed, all discussions resolved
victoria-mcgrath
approved these changes
Nov 23, 2021
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @igchor)
Revert some of the changes made earlier and reimplement them without chaning getMemoryTierConfigs(). After this change, it is no longer possible to call setCacheSize() and configureMemoryTiers() in arbitrary order - setCacheSize() must be called first.
vinser52
added a commit
to vinser52/CacheLib
that referenced
this pull request
Sep 13, 2022
Initial support of NUMA bindings
danielobiri
pushed a commit
to danielobiri/CacheLib
that referenced
this pull request
Oct 20, 2022
Initial support of NUMA bindings
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.
Revert some of the changes made earlier and reimplement them without
chaning getMemoryTierConfigs().
After this change, it is no longer possible to call setCacheSize()
and configureMemoryTiers() in arbitrary order - setCacheSize() must
be called first.
I'm not 100% sure if this way is better because if we introduce some new global setting which we would like to apply for all tiers (like currently usePosixShm) we will have to do that both in configureMemoryTiers and in function setting this property (see usePosixShm for example). Currently, we can do that just once, in getMemoryTierConfigs()
This change is