pmem / CacheLib

Pluggable in-process caching engine to build and scale high performance services
https://www.cachelib.org
Apache License 2.0
5 stars 13 forks source link

Config api integration #2

Closed igchor closed 2 years ago

igchor commented 2 years ago

Right now, it only works when Allocator is constructed using shared memory (constructor taking SharedMemNew or SharedMemAttach)

TODO:


This change is Reviewable

victoria-mcgrath commented 2 years ago

cachelib/allocator/tests/BaseAllocatorTest.h, line 1153 at r4 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…
Right, this is the question - do we want to allow passing empty vector to configure? I think not. Ideally we would have one additional MemoryTierCacheConfig which would correspond to legacy behavior (posix/sysv) and we could use that as a default parameter. What do you think?

Yes, I like the idea of default parameter to support legacy mode. As for the check, originally I was thinking that we could quietly return from configure on an empty vector, but after thinking more, I suggest throwing an exception in this case because an empty vector may be passed by mistake. What do you think?

victoria-mcgrath commented 2 years ago

cachelib/allocator/CacheAllocator-inl.h, line 166 at r4 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…
Done. Actually I tihnk we should implement it in this way(in later PR perhaps): - introduce MemoryTierCacheConfig::fromPosixSysV(bool posix) - initialize memoryTierConfigs in AllocatorConfig to contain this posix/sysv MemoryTierCacheConfig This would allow us to remove this check for size and hide everything inside config. @victoria-mcgrath what do you think?

Yes, let's do this.

victoria-mcgrath commented 2 years ago

cachelib/allocator/CacheAllocatorConfig.h, line 584 at r6 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…
But if user defined all tiers with ratio then it's OK to call setCacheSize, right?

Yeah, if you check all of them and they are all ratios then it should be fine. What do you think about setting size for the default tier here?

victoria-mcgrath commented 2 years ago

cachelib/allocator/CacheAllocatorConfig.h, line 849 at r6 (raw file):

setUsePosixForShm Why do you need to synchronize with setUsePosixForShm?

igchor commented 2 years ago

All tests (which we run on CI) have passed: https://github.com/pmem/CacheLib/pull/8