Skip to content

Commit

Permalink
accounts-index: remove in-memory account index limit (#2721)
Browse files Browse the repository at this point in the history 
* panic when exceed in-mem index budget

* remove in-memory account index limit

* add index limit arg to deprecated arg list

* fix a test

* pr reviews

* add copy for IndexLimitMb enum to allow unwrap_or_default

---------

Co-authored-by: HaoranYi <[email protected]>
  • Loading branch information
@HaoranYi
HaoranYi and HaoranYi authored Aug 28, 2024
1 parent c8fbfe7 commit b6c3d50
Show file tree
Hide file tree
Showing 6 changed files with 40 additions and 155 deletions.
22 changes: 8 additions & 14 deletions accounts-db/src/accounts_index.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ pub const ACCOUNTS_INDEX_CONFIG_FOR_TESTING: AccountsIndexConfig = AccountsIndex
bins: Some(BINS_FOR_TESTING),
flush_threads: Some(FLUSH_THREADS_TESTING),
drives: None,
index_limit_mb: IndexLimitMb::Unspecified,
index_limit_mb: IndexLimitMb::Unlimited,
ages_to_stay_in_cache: None,
scan_results_limit_bytes: None,
started_from_validator: false,
Expand All @@ -59,7 +59,7 @@ pub const ACCOUNTS_INDEX_CONFIG_FOR_BENCHMARKS: AccountsIndexConfig = AccountsIn
bins: Some(BINS_FOR_BENCHMARKS),
flush_threads: Some(FLUSH_THREADS_TESTING),
drives: None,
index_limit_mb: IndexLimitMb::Unspecified,
index_limit_mb: IndexLimitMb::Unlimited,
ages_to_stay_in_cache: None,
scan_results_limit_bytes: None,
started_from_validator: false,
Expand Down Expand Up @@ -189,22 +189,16 @@ pub struct AccountSecondaryIndexesIncludeExclude {
}

/// specification of how much memory in-mem portion of account index can use
#[derive(Debug, Clone)]
#[derive(Debug, Copy, Clone, Default)]
pub enum IndexLimitMb {
/// nothing explicit specified, so default
Unspecified,
/// limit was specified, use disk index for rest
Limit(usize),
/// use disk index while allowing to use as much memory as available for
/// in-memory index.
#[default]
Unlimited,
/// in-mem-only was specified, no disk index
InMemOnly,
}

impl Default for IndexLimitMb {
fn default() -> Self {
Self::Unspecified
}
}

#[derive(Debug, Default, Clone)]
pub struct AccountsIndexConfig {
pub bins: Option<usize>,
Expand Down Expand Up @@ -2487,7 +2481,7 @@ pub mod tests {

let mut config = ACCOUNTS_INDEX_CONFIG_FOR_TESTING;
config.index_limit_mb = if use_disk {
IndexLimitMb::Limit(10_000)
IndexLimitMb::Unlimited
} else {
IndexLimitMb::InMemOnly // in-mem only
};
Expand Down
62 changes: 3 additions & 59 deletions accounts-db/src/accounts_index/in_mem_accounts_index.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -980,13 +980,6 @@ impl<T: IndexValue, U: DiskIndexValue + From<T> + Into<T>> InMemAccountsIndex<T,
thread_rng().gen_range(0..N) == 0
}

/// assumes 1 entry in the slot list. Ignores overhead of the HashMap and such
fn approx_size_of_one_entry() -> usize {
std::mem::size_of::<T>()
+ std::mem::size_of::<Pubkey>()
+ std::mem::size_of::<AccountMapEntry<T>>()
}

fn should_evict_based_on_age(
current_age: Age,
entry: &AccountMapEntry<T>,
Expand All @@ -1003,16 +996,12 @@ impl<T: IndexValue, U: DiskIndexValue + From<T> + Into<T>> InMemAccountsIndex<T,
entry: &'a AccountMapEntry<T>,
startup: bool,
update_stats: bool,
exceeds_budget: bool,
ages_flushing_now: Age,
) -> (bool, Option<std::sync::RwLockReadGuard<'a, SlotList<T>>>) {
// this could be tunable dynamically based on memory pressure
// we could look at more ages or we could throw out more items we are choosing to keep in the cache
if Self::should_evict_based_on_age(current_age, entry, startup, ages_flushing_now) {
if exceeds_budget {
// if we are already holding too many items in-mem, then we need to be more aggressive at kicking things out
(true, None)
} else if entry.ref_count() != 1 {
if entry.ref_count() != 1 {
Self::update_stat(&self.stats().held_in_mem.ref_count, 1);
(false, None)
} else {
Expand Down Expand Up @@ -1199,7 +1188,6 @@ impl<T: IndexValue, U: DiskIndexValue + From<T> + Into<T>> InMemAccountsIndex<T,
if !evictions_age_possible.is_empty() || !evictions_random.is_empty() {
let disk = self.bucket.as_ref().unwrap();
let mut flush_entries_updated_on_disk = 0;
let exceeds_budget = self.get_exceeds_budget();
let mut flush_should_evict_us = 0;
// we don't care about lock time in this metric - bg threads can wait
let m = Measure::start("flush_update");
Expand All @@ -1218,7 +1206,6 @@ impl<T: IndexValue, U: DiskIndexValue + From<T> + Into<T>> InMemAccountsIndex<T,
&v,
startup,
true,
exceeds_budget,
ages_flushing_now,
);
slot_list = slot_list_temp;
Expand Down Expand Up @@ -1331,21 +1318,6 @@ impl<T: IndexValue, U: DiskIndexValue + From<T> + Into<T>> InMemAccountsIndex<T,
}
}

/// calculate the estimated size of the in-mem index
/// return whether the size exceeds the specified budget
fn get_exceeds_budget(&self) -> bool {
let in_mem_count = self.stats().count_in_mem.load(Ordering::Relaxed);
let limit = self.storage.mem_budget_mb;
let estimate_mem = in_mem_count * Self::approx_size_of_one_entry();
let exceeds_budget = limit
.map(|limit| estimate_mem >= limit * 1024 * 1024)
.unwrap_or_default();
self.stats()
.estimate_mem
.store(estimate_mem as u64, Ordering::Relaxed);
exceeds_budget
}

/// for each key in 'keys', look up in map, set age to the future
fn move_ages_to_future(&self, next_age: Age, current_age: Age, keys: &[Pubkey]) {
let map = self.map_internal.read().unwrap();
Expand Down Expand Up @@ -1564,7 +1536,7 @@ impl Drop for EvictionsGuard<'_> {
mod tests {
use {
super::*,
crate::accounts_index::{AccountsIndexConfig, IndexLimitMb, BINS_FOR_TESTING},
crate::accounts_index::{AccountsIndexConfig, BINS_FOR_TESTING},
assert_matches::assert_matches,
itertools::Itertools,
};
Expand All @@ -1582,10 +1554,7 @@ mod tests {
fn new_disk_buckets_for_test<T: IndexValue>() -> InMemAccountsIndex<T, T> {
let holder = Arc::new(BucketMapHolder::new(
BINS_FOR_TESTING,
&Some(AccountsIndexConfig {
index_limit_mb: IndexLimitMb::Limit(1),
..AccountsIndexConfig::default()
}),
&Some(AccountsIndexConfig::default()),
1,
));
let bin = 0;
Expand Down Expand Up @@ -1615,7 +1584,6 @@ mod tests {
&one_element_slot_list_entry,
startup,
false,
false,
1,
)
.0,
Expand Down Expand Up @@ -1695,23 +1663,6 @@ mod tests {
AccountMapEntryMeta::default(),
));

// exceeded budget
assert!(
bucket
.should_evict_from_mem(
current_age,
&Arc::new(AccountMapEntryInner::new(
vec![],
ref_count,
AccountMapEntryMeta::default()
)),
startup,
false,
true,
0,
)
.0
);
// empty slot list
assert!(
!bucket
Expand All @@ -1724,7 +1675,6 @@ mod tests {
)),
startup,
false,
false,
0,
)
.0
Expand All @@ -1737,7 +1687,6 @@ mod tests {
&one_element_slot_list_entry,
startup,
false,
false,
0,
)
.0
Expand All @@ -1754,7 +1703,6 @@ mod tests {
)),
startup,
false,
false,
0,
)
.0
Expand All @@ -1774,7 +1722,6 @@ mod tests {
)),
startup,
false,
false,
0,
)
.0
Expand All @@ -1789,7 +1736,6 @@ mod tests {
&one_element_slot_list_entry,
startup,
false,
false,
0,
)
.0
Expand All @@ -1804,7 +1750,6 @@ mod tests {
&one_element_slot_list_entry,
startup,
false,
false,
0,
)
.0
Expand All @@ -1819,7 +1764,6 @@ mod tests {
&one_element_slot_list_entry,
startup,
false,
false,
0,
)
.0
Expand Down
50 changes: 6 additions & 44 deletions accounts-db/src/bucket_map_holder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,6 @@ const _: () = assert!(std::mem::size_of::<Age>() == std::mem::size_of::<AtomicAg

const AGE_MS: u64 = DEFAULT_MS_PER_SLOT; // match one age per slot time

// 10 GB limit for in-mem idx. In practice, we don't get this high. This tunes how aggressively to save items we expect to use soon.
pub const DEFAULT_DISK_INDEX: Option<usize> = Some(10_000);

pub struct BucketMapHolder<T: IndexValue, U: DiskIndexValue + From<T> + Into<T>> {
pub disk: Option<BucketMap<(Slot, U)>>,

Expand Down Expand Up @@ -59,10 +56,6 @@ pub struct BucketMapHolder<T: IndexValue, U: DiskIndexValue + From<T> + Into<T>>

pub threads: usize,

// how much mb are we allowed to keep in the in-mem index?
// Rest goes to disk.
pub mem_budget_mb: Option<usize>,

/// how many ages should elapse from the last time an item is used where the item will remain in the cache
pub ages_to_stay_in_cache: Age,

Expand Down Expand Up @@ -217,43 +210,15 @@ impl<T: IndexValue, U: DiskIndexValue + From<T> + Into<T>> BucketMapHolder<T, U>
config.drives.clone()
});

let mem_budget_mb = match config
let disk = match config
.as_ref()
.map(|config| &config.index_limit_mb)
.unwrap_or(&IndexLimitMb::Unspecified)
.map(|config| config.index_limit_mb)
.unwrap_or_default()
{
// creator said to use disk idx with a specific limit
IndexLimitMb::Limit(mb) => Some(*mb),
// creator said InMemOnly, so no disk index
IndexLimitMb::InMemOnly => None,
// whatever started us didn't specify whether to use the acct idx
IndexLimitMb::Unspecified => {
// check env var if we were not started from a validator
let mut use_default = true;
if !config
.as_ref()
.map(|config| config.started_from_validator)
.unwrap_or_default()
{
if let Ok(_limit) = std::env::var("SOLANA_TEST_ACCOUNTS_INDEX_MEMORY_LIMIT_MB")
{
// Note this env var means the opposite of the default. The default now is disk index is on.
// So, if this env var is set, DO NOT allocate with disk buckets if mem budget was not set, we were NOT started from validator, and env var was set
// we do not want the env var to have an effect when running the validator (only tests, benches, etc.)
use_default = false;
}
}
if use_default {
// if validator does not specify disk index limit or specify in mem only, then this is the default
DEFAULT_DISK_INDEX
} else {
None
}
}
IndexLimitMb::Unlimited => Some(BucketMap::new(bucket_config)),
};

// only allocate if mem_budget_mb is Some
let disk = mem_budget_mb.map(|_| BucketMap::new(bucket_config));
Self {
disk,
ages_to_stay_in_cache,
Expand All @@ -270,7 +235,7 @@ impl<T: IndexValue, U: DiskIndexValue + From<T> + Into<T>> BucketMapHolder<T, U>
age_timer: AtomicInterval::default(),
bins,
startup: AtomicBool::default(),
mem_budget_mb,

threads,
_phantom: PhantomData,
startup_stats: Arc::default(),
Expand Down Expand Up @@ -512,10 +477,7 @@ pub mod tests {
#[test]
fn test_disk_index_enabled() {
let bins = 1;
let config = AccountsIndexConfig {
index_limit_mb: IndexLimitMb::Limit(0),
..AccountsIndexConfig::default()
};
let config = AccountsIndexConfig::default();
let test = BucketMapHolder::<u64, u64>::new(bins, &Some(config), 1);
assert!(test.is_disk_index_enabled());
}
Expand Down
25 changes: 6 additions & 19 deletions ledger-tool/src/args.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,22 +59,12 @@ pub fn accounts_db_args<'a, 'b>() -> Box<[Arg<'a, 'b>]> {
.validator(is_pow2)
.takes_value(true)
.help("Number of bins to divide the accounts index into"),
Arg::with_name("accounts_index_memory_limit_mb")
.long("accounts-index-memory-limit-mb")
.value_name("MEGABYTES")
.validator(is_parsable::<usize>)
.takes_value(true)
.help(
"How much memory the accounts index can consume. If this is exceeded, some \
account index entries will be stored on disk.",
),
Arg::with_name("disable_accounts_disk_index")
.long("disable-accounts-disk-index")
.help(
"Disable the disk-based accounts index. It is enabled by default. The entire \
accounts index will be kept in memory.",
)
.conflicts_with("accounts_index_memory_limit_mb"),
),
Arg::with_name("accounts_db_skip_shrink")
.long("accounts-db-skip-shrink")
.help(
Expand Down Expand Up @@ -241,14 +231,11 @@ pub fn get_accounts_db_config(
let ledger_tool_ledger_path = ledger_path.join(LEDGER_TOOL_DIRECTORY);

let accounts_index_bins = value_t!(arg_matches, "accounts_index_bins", usize).ok();
let accounts_index_index_limit_mb =
if let Ok(limit) = value_t!(arg_matches, "accounts_index_memory_limit_mb", usize) {
IndexLimitMb::Limit(limit)
} else if arg_matches.is_present("disable_accounts_disk_index") {
IndexLimitMb::InMemOnly
} else {
IndexLimitMb::Unspecified
};
let accounts_index_index_limit_mb = if arg_matches.is_present("disable_accounts_disk_index") {
IndexLimitMb::InMemOnly
} else {
IndexLimitMb::Unlimited
};
let accounts_index_drives = values_t!(arg_matches, "accounts_index_path", String)
.ok()
.map(|drives| drives.into_iter().map(PathBuf::from).collect())
Expand Down
Loading

0 comments on commit b6c3d50

Please sign in to comment.