Skip to content

Background evictor thread #52

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 16 commits into from
Closed

Conversation

byrnedj
Copy link
Collaborator

@byrnedj byrnedj commented Mar 3, 2022

Purpose – To increase insertion path performance, we would like to pre-emptively evict items from the current tier so that allocations may be done without needing to call an eviction.

Method – Implement background thread performing eviction for a given tier (right now this is tier 0 by default but can be extended in the future) – for each memory pool we check the classes to see if the class exceeds a preconfigured free space threshold – if the class does exceed the threshold, we calculate the number of items needed to evict in order to meet that threshold. This way the evictions occur in batch for each class – avoiding contending for the LRU lock for individual evictions.

Results

== Test Results Without Background Evictor Thread ==
== Allocator Stats ==
Items in RAM : 115,951
Items in NVM : 0
Alloc Attempts: 4,311,097 Success: 84.89%
RAM Evictions : 3,518,361
Background Tier 0 Evictions : 0
Background Tier 0 Eviction Runs : 0
Cache Gets : 4,000,000
Hit Ratio : 44.66%
NVM Gets : 0, Coalesced : 100.00%
NVM Puts : 0, Success : 0.00%, Clean : 100.00%, AbortsFromDel : 0, AbortsFromGet : 0
NVM Evicts : 0, Clean : 100.00%, Unclean : 0, Double : 0
NVM Deletes : 0 Skipped Deletes: 100.00%

== Throughput for ==
Total Ops : 4.00 million
Total sets: 2,213,563
get : 169,688/s, success : 44.66%
set : 93,903/s, success : 100.00%
del : 0/s, found : 0.00%

== Test Results With Background Eviction Running every 100ms - 100 allocations per class free ==
== Allocator Stats ==
Items in RAM : 28,634
Items in NVM : 0
Alloc Attempts: 4,422,553 Success: 63.16%
RAM Evictions : 2,657,768
Background Tier 0 Evictions : 87,098
Background Tier 0 Eviction Runs : 148
Cache Gets : 4,000,000
Hit Ratio : 44.36%
NVM Gets : 0, Coalesced : 100.00%
NVM Puts : 0, Success : 0.00%, Clean : 100.00%, AbortsFromDel : 0, AbortsFromGet : 0
NVM Evicts : 0, Clean : 100.00%, Unclean : 0, Double : 0
NVM Deletes : 0 Skipped Deletes: 100.00%

== Throughput for ==
Total Ops : 4.00 million
Total sets: 2,225,607
get : 224,013/s, success : 44.36%
set : 124,641/s, success : 100.00%
del : 0/s, found : 0.00%

Improvement in GET/SET throughput is about 30% for this small test (200MB cache, 4M requests, 400K objects, normal distribution, 8 threads) - see attached configs.
config-4GB-DRAM-4GB-DRAM-short.txt
config-4GB-DRAM-4GB-DRAM-short-bg-evict.txt


This change is Reviewable

@vinser52
Copy link
Collaborator

vinser52 commented Mar 9, 2022

Could you please also run your tests with our normal 4GB-DRAM-4GB-PMEM config?

@byrnedj
Copy link
Collaborator Author

byrnedj commented Mar 14, 2022

Updated results with the normal config after fix with item's not being properly released. About 30% improvement in throughput.
bg_10ms_0.001_post.txt
no_bg_post.txt

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 18 files reviewed, 1 unresolved discussion (waiting on @byrnedj and @igchor)


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

                          config_.ccacheOptimizeStepSizePercent);
  }
  if (config_.poolOptimizerEnabled()) {

isn't this a duplicate of code above?


cachelib/allocator/FreeThresholdStrategy.cpp, line 39 at r2 (raw file):

  size_t freeMem = mpStats.acStats.at(cid).getTotalFreeMemory() / allocSize;

  if (totalMem > 0) {

nit: probably you can just replace this whole function with: return (double)mpStats.acStats.at(cid).getTotalFreeMemory() / (double)mpStats.acStats.at(cid).getTotalMemory()< freeThreshold_;

You don't even need to divide by allocSize, since you have it on both sides of division operator

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 18 files reviewed, 2 unresolved discussions (waiting on @byrnedj and @igchor)


cachelib/allocator/KeepFreeStrategy.cpp, line 37 at r2 (raw file):

    size_t totalAllocs = mpStats.acStats.at(cid).getTotalMemory() / allocSize;
    size_t freeAllocs = mpStats.acStats.at(cid).getTotalFreeMemory() / allocSize;
    if (totalAllocs > nKeepFree_ && freeAllocs < nKeepFree_) {

why not just freeAllocs < nKeepFree_?

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.

@byrnedj I think there is a compile error in some test, could you please fix it? This is because you added new function (getPoolById) and did not modify tests.

Reviewable status: 0 of 18 files reviewed, 2 unresolved discussions (waiting on @byrnedj and @igchor)

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 20 files reviewed, 3 unresolved discussions (waiting on @byrnedj and @igchor)


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

  } else {
      //when an eviction for a given pid,cid at tier 0 is triggered this will be run
      while (1) {

why is this infinite loop?

@igchor
Copy link

igchor commented Jul 5, 2022

Now, part of: #90

@igchor igchor closed this Jul 5, 2022
guptask added a commit to guptask/CacheLib that referenced this pull request Feb 21, 2023
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.

4 participants