Use RWLock for shrink map as map should be small, and dashmap is empty checks are expensive

[roryharr]

Problem

Empty checks on dashmaps are expensive

Summary of Changes

Since shrink map is relatively small, replace with RwLock

Grabbed a quick check of clean timings:

The line of interest is the blue line. Others are just reference lines. There are three different versions running on the blue line:

1. 1st restart is with the RwLock
2. 2nd restart is with the Base code Dashmap
   3 3rd restart is with the RwLock and removing all the asserts

1 & 3 seem noticeably better. I don't see a real difference between 1 and 3, so I am proposing the RwLock to keep the asserts.

There are many other paths effected by the empty check, but they have more noise than the clean flow.

Fixes #

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.2%. Comparing base (6f29aeb) to head (f30dfeb).
Report is 15 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #7007     +/-   ##
=========================================
- Coverage    83.2%    83.2%   -0.1%     
=========================================
  Files         853      853             
  Lines      375393   375427     +34     
=========================================
+ Hits       312520   312523      +3     
- Misses      62873    62904     +31     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[brooksprumo]

Here's the edge canaries over the last 24 hours for max number of shrunk storages at a time, which would be the number of entries in the shrink_in_progress_map. My thought being that the usefulness of DashMap increases as the number of entries increases.

The "normal" max is between 200 and 400. The big spike is at the epoch boundary, above 3,000. I imagine the single HashMap is probably fine for these values? Can you also check the shrink times with your PR, esp around the epoch boundary?

[brooksprumo]

Also, can you mark the PR Ready for Review and add @HaoranYi as a reviewer, please?

[roryharr]

Here's the edge canaries over the last 24 hours for max number of shrunk storages at a time, which would be the number of entries in the shrink_in_progress_map. My thought being that the usefulness of DashMap increases as the number of entries increases.

The "normal" max is between 200 and 400. The big spike is at the epoch boundary, above 3,000. I imagine the single HashMap is probably fine for these values? Can you also check the shrink times with your PR, esp around the epoch boundary?

Shrink Times:

Similar to the clean graph:

1. First restart around 1730 is with rwlock hashmap
2. Second restart around 2100 is baselne
3. Third restart around 2300 is no empty check

Looks like a decent improvement in shrink as well

[roryharr]

Also, can you mark the PR Ready for Review and add @HaoranYi as a reviewer, please?

Ha, sorry. I had requested it for review and then reverted it to draft and removed reviewers as I wanted to gather more data

[brooksprumo]

[HaoranYi]

lgtm.

Do you have timing for shrink? The original dashmap will allow concurrent access between clean and shrink as long as they are looking up in different shards. Rwlock will prevent all concurrent access. It is likely that shrink won't be affected since it is not a big map. Just want to double check to make sure.

[roryharr]

lgtm.

Do you have timing for shrink? The original dashmap will allow concurrent access between clean and shrink as long as they are looking up in different shards. Rwlock will prevent all concurrent access. It is likely that shrink won't be affected since it is not a big map. Just want to double check to make sure.

Yup! posted it above:

Similar to the clean graph:

First restart around 1730 is with rwlock hashmap
Second restart around 2100 is baselne
Third restart around 2300 is no empty check

Looks like a decent improvement in shrink as well