Re-use accounts stores#12885
Conversation
sakridge
force-pushed
the
re-use-account-stores
branch
2 times, most recently
from
b464577 to
c0d7a73
Compare
|
|
||
| pub fn slot(&self) -> Slot { | ||
| self.slot | ||
| self.slot.load(Ordering::Acquire) |
There was a problem hiding this comment.
Our first acquire-release semantics! To celebrate maybe we should include a comment about why it's necessary here?
There was a problem hiding this comment.
I think we use it in gossip as well. Most of our atomics are not critical for functionality like perf counters which are fine if they are incremented or loaded out of order. I thought it would be bad if the load/store is re-ordered for the slot value, a thread could see a stale slot value.
There was a problem hiding this comment.
Just to clarify my understanding and future reference about the interaction between the atomics slot and id fields in AccountStorageEntry:
A recycle scenario can be summarized with something like the following:
Initial state:
self.slot = 0
self.id = 0
Thread A grabs and holds a copy of the storage entry Arc and reads
Thread A
let ref_count = storage.lookup(store_id_x); // lookup in the storage for some store_id_x, may be the old store id, or the new recycled id
ref_count.store(2, Ordering::SeqCst); // storage clone bumps the Arc refcount, Arc uses `SeqCst`
slot.load(Ordering::Acquire);
id.load(Ordering::Relaxed);
ref_count.store(1, Ordering::SeqCst);
Thread B performing the recycle:
Thread B
// do the recycle
if refcount.load(Ordering::SeqCst) == 1 { // Arc `strong_count()` uses `SeqCst`
slot.store(5, Ordering::Release);
id.store(5, Ordering::Relaxed);
}
// Insert the store
storage.lock().insert(store_id_y); // insert recycled store into storage with new store id
So now looking at possible combinations of (slot, id) that thread A can see:
-
If Thread
Ais reading directly grabbing and holding copy of theArcfor the storage entry from therecycled_stores, then it can see any combination of (0, 0), (0, 5), (5, 5) (5, 0), because the loads can interleave with the recycle logic inBthat starts atif Arc::strong_count(store) == 1 { -
If Thread
Ais finding the storage entry through a read onself.storages:
Can threadAstill see a mixed combination like (0, 5), or (5, 0)? I think the storage lock and thestrong_count() == 1check prevent this from happening?
For example for(5, 0), this meansAmust have seenB's store toslot, which means store_id_x == store_id_y,
so the loads in threadAmust have occurred after the new storage withstore_id_ywas inserted into the store atstorage.lock().insert(store_id_y);. And because locks have full fences, both loads in threadAcannot be ordered before thestorage.lock().insert(store_id_y);, and must see both stores from threadB.
(0, 5)follows a similar reasoning.
sakridge
force-pushed
the
re-use-account-stores
branch
4 times, most recently
from
33fbd71 to
52ea4ac
Compare
sakridge
force-pushed
the
re-use-account-stores
branch
5 times, most recently
from
c6eb905 to
9204156
Compare
|
Comparison of find storage time, new is on left. After warmup, it's improved from 10-15ms to 2-4ms.
|
sakridge
force-pushed
the
re-use-account-stores
branch
from
9204156 to
fd5e992
Compare
Codecov Report
@@ Coverage Diff @@
## master #12885 +/- ##
========================================
Coverage 82.0% 82.0%
========================================
Files 376 376
Lines 87885 88114 +229
========================================
+ Hits 72117 72322 +205
- Misses 15768 15792 +24 |
sakridge
force-pushed
the
re-use-account-stores
branch
from
fd5e992 to
8a3e92d
Compare
Creating files and dropping mmap areas can be expensive
Can encounter an infinite loop when the store is too small, but smaller than the normal store size.
sakridge
force-pushed
the
re-use-account-stores
branch
from
8a3e92d to
c1b9f1f
Compare
| if !self.has_space_available(slot, data_len) { | ||
| let special_store_size = std::cmp::max(data_len * 2, self.file_size); | ||
| if self | ||
| .try_recycle_and_insert_store(slot, special_store_size, std::u64::MAX) |
There was a problem hiding this comment.
nit: Should we wrap this call to try_recycle_and_insert_store in a Measure and be add it to the self.stats.store_find_store timing metric?
* Re-use accounts_db stores Creating files and dropping mmap areas can be expensive * Add test for storage finder Can encounter an infinite loop when the store is too small, but smaller than the normal store size. * Fix storage finding * Check for strong_count == 1 * try_recycle helper
* Re-use accounts_db stores Creating files and dropping mmap areas can be expensive * Add test for storage finder Can encounter an infinite loop when the store is too small, but smaller than the normal store size. * Fix storage finding * Check for strong_count == 1 * try_recycle helper
This reverts commit 43053dc.
* Re-use accounts_db stores Creating files and dropping mmap areas can be expensive * Add test for storage finder Can encounter an infinite loop when the store is too small, but smaller than the normal store size. * Fix storage finding * Check for strong_count == 1 * try_recycle helper
* Re-use accounts_db stores Creating files and dropping mmap areas can be expensive * Add test for storage finder Can encounter an infinite loop when the store is too small, but smaller than the normal store size. * Fix storage finding * Check for strong_count == 1 * try_recycle helper
* Re-use accounts_db stores Creating files and dropping mmap areas can be expensive * Add test for storage finder Can encounter an infinite loop when the store is too small, but smaller than the normal store size. * Fix storage finding * Check for strong_count == 1 * try_recycle helper
3 participants
Problem
File/mmap creation can be expensive (100s of ms) and mmap deletion causes a flush to disk which increases unnecessary writes to disk when that data is not actually needed.
Summary of Changes
Save stores for re-use later.
Fixes #