Skip to content
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.

Sign up for GitHub

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

v1.16: Shares accounts hash cache data between full and incremental (backport of #33164) #33212

Merged
merged 2 commits into from
Sep 13, 2023
Merged

v1.16: Shares accounts hash cache data between full and incremental (backport of #33164) #33212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f2e100c
Shares accounts hash cache data between full and incremental (#33164)
brooksprumo Sep 11, 2023
d542486
fix merge conflicts
brooksprumo Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 18 additions & 17 deletions runtime/src/accounts_db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1396,9 +1396,7 @@ pub struct AccountsDb {
/// Set of storage paths to pick from
pub(crate) paths: Vec<PathBuf>,

/// Directories for account hash calculations, within base_working_path
full_accounts_hash_cache_path: PathBuf,
incremental_accounts_hash_cache_path: PathBuf,
accounts_hash_cache_path: PathBuf,
transient_accounts_hash_cache_path: PathBuf,

// used by tests
Expand Down Expand Up @@ -2370,7 +2368,7 @@ impl AccountsDb {
(accounts_hash_cache_path, None)
} else {
let temp_dir = TempDir::new().expect("new tempdir");
let cache_path = temp_dir.path().join(Self::DEFAULT_ACCOUNTS_HASH_CACHE_DIR);
let cache_path = temp_dir.path().to_path_buf();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This temp dir is only used by the accounts hash cache. By using it directly, we don't need to create a subdir within it.

(cache_path, Some(temp_dir))
};

Expand Down Expand Up @@ -2402,9 +2400,8 @@ impl AccountsDb {
write_cache_limit_bytes: None,
write_version: AtomicU64::new(0),
paths: vec![],
full_accounts_hash_cache_path: accounts_hash_cache_path.join("full"),
incremental_accounts_hash_cache_path: accounts_hash_cache_path.join("incremental"),
transient_accounts_hash_cache_path: accounts_hash_cache_path.join("transient"),
accounts_hash_cache_path,
temp_accounts_hash_cache_path,
shrink_paths: RwLock::new(None),
temp_paths: None,
Expand Down Expand Up @@ -7517,18 +7514,23 @@ impl AccountsDb {
fn get_cache_hash_data(
accounts_hash_cache_path: PathBuf,
config: &CalcAccountsHashConfig<'_>,
flavor: CalcAccountsHashFlavor,
slot: Slot,
) -> CacheHashData {
if !config.store_detailed_debug_info_on_failure {
CacheHashData::new(accounts_hash_cache_path)
let accounts_hash_cache_path = if !config.store_detailed_debug_info_on_failure {
accounts_hash_cache_path
} else {
// this path executes when we are failing with a hash mismatch
let failed_dir = accounts_hash_cache_path
.join("failed_calculate_accounts_hash_cache")
.join(slot.to_string());
let _ = std::fs::remove_dir_all(&failed_dir);
CacheHashData::new(failed_dir)
}
_ = std::fs::remove_dir_all(&failed_dir);
failed_dir
};
CacheHashData::new(
accounts_hash_cache_path,
flavor == CalcAccountsHashFlavor::Full,
)
}

// modeled after calculate_accounts_delta_hash
Expand All @@ -7544,7 +7546,6 @@ impl AccountsDb {
storages,
stats,
CalcAccountsHashFlavor::Full,
self.full_accounts_hash_cache_path.clone(),
)?;
let AccountsHashEnum::Full(accounts_hash) = accounts_hash else {
panic!("calculate_accounts_hash_from_storages must return a FullAccountsHash");
Expand Down Expand Up @@ -7572,7 +7573,6 @@ impl AccountsDb {
storages,
stats,
CalcAccountsHashFlavor::Incremental,
self.incremental_accounts_hash_cache_path.clone(),
)?;
let AccountsHashEnum::Incremental(incremental_accounts_hash) = accounts_hash else {
panic!("calculate_incremental_accounts_hash must return an IncrementalAccountsHash");
Expand All @@ -7586,7 +7586,6 @@ impl AccountsDb {
storages: &SortedStorages<'_>,
mut stats: HashStats,
flavor: CalcAccountsHashFlavor,
accounts_hash_cache_path: PathBuf,
) -> Result<(AccountsHashEnum, u64), AccountsHashVerificationError> {
let _guard = self.active_stats.activate(ActiveStatItem::Hash);
stats.oldest_root = storages.range().start;
Expand All @@ -7595,8 +7594,10 @@ impl AccountsDb {

let slot = storages.max_slot_inclusive();
let use_bg_thread_pool = config.use_bg_thread_pool;
let accounts_hash_cache_path = self.accounts_hash_cache_path.clone();
let scan_and_hash = || {
let cache_hash_data = Self::get_cache_hash_data(accounts_hash_cache_path, config, slot);
let cache_hash_data =
Self::get_cache_hash_data(accounts_hash_cache_path, config, flavor, slot);

let bounds = Range {
start: 0,
Expand Down Expand Up @@ -9592,7 +9593,7 @@ pub mod tests {
let temp_dir = TempDir::new().unwrap();
let accounts_hash_cache_path = temp_dir.path().to_path_buf();
self.scan_snapshot_stores_with_cache(
&CacheHashData::new(accounts_hash_cache_path),
&CacheHashData::new(accounts_hash_cache_path, true),
storage,
stats,
bins,
Expand Down Expand Up @@ -10691,7 +10692,7 @@ pub mod tests {
};

let result = accounts_db.scan_account_storage_no_bank(
&CacheHashData::new(accounts_hash_cache_path),
&CacheHashData::new(accounts_hash_cache_path, true),
&CalcAccountsHashConfig::default(),
&get_storage_refs(&[storage]),
test_scan,
Expand Down
15 changes: 9 additions & 6 deletions runtime/src/cache_hash_data.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,29 +140,32 @@ impl CacheHashDataFile {
}
}

pub type PreExistingCacheFiles = HashSet<PathBuf>;
pub struct CacheHashData {
cache_dir: PathBuf,
pre_existing_cache_files: Arc<Mutex<PreExistingCacheFiles>>,
pre_existing_cache_files: Arc<Mutex<HashSet<PathBuf>>>,
should_delete_old_cache_files_on_drop: bool,
Comment on lines +145 to +146
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the inconsistent terminology here isn't great "old" vs. "pre-existing"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the inconsistent terminology here isn't great "old" vs. "pre-existing"

Yes, it's non-intuitive.

When an instance of CacheHashData is created, we call get_cache_files() on it. That method populates pre_existing_cache_files will all the cache files it finds on disk. As this instance gets used, it is queried for cache files via get_file_reference_to_map_later(). As part of that function, we remove the path from the pre_existing_cache_files.

So, on drop, the remaining paths left in pre_existing_cache_files are all the ones that have not been used. This indicates they are now old, since they were not part of the accounts hash calculation, and can be deleted.

So the tl;dr is that on drop, "pre-existing" means "old", which is why both the method (delete_old_cache_files()) and the flag (should_delete_old_cache_files_on_drop) have "old" in the name.

We could tweak that names to reflect this, or add documentation, or a second HashSet/Container for the pre-existing vs the old files. I would image that those changes should go to master first.

pub stats: Arc<Mutex<CacheHashDataStats>>,
}

impl Drop for CacheHashData {
fn drop(&mut self) {
self.delete_old_cache_files();
if self.should_delete_old_cache_files_on_drop {
self.delete_old_cache_files();
}
self.stats.lock().unwrap().report();
}
}

impl CacheHashData {
pub fn new(cache_dir: PathBuf) -> CacheHashData {
pub fn new(cache_dir: PathBuf, should_delete_old_cache_files_on_drop: bool) -> CacheHashData {
std::fs::create_dir_all(&cache_dir).unwrap_or_else(|err| {
panic!("error creating cache dir {}: {err}", cache_dir.display())
});

let result = CacheHashData {
cache_dir,
pre_existing_cache_files: Arc::new(Mutex::new(PreExistingCacheFiles::default())),
pre_existing_cache_files: Arc::new(Mutex::new(HashSet::default())),
should_delete_old_cache_files_on_drop,
stats: Arc::new(Mutex::new(CacheHashDataStats::default())),
};

Expand Down Expand Up @@ -386,7 +389,7 @@ pub mod tests {
data_this_pass.push(this_bin_data);
}
}
let cache = CacheHashData::new(cache_dir.clone());
let cache = CacheHashData::new(cache_dir.clone(), true);
let file_name = PathBuf::from("test");
cache.save(&file_name, &data_this_pass).unwrap();
cache.get_cache_files();
Expand Down