Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Removes unnecessary Accounts constructors #34471

Merged
merged 4 commits into from
Dec 15, 2023
Merged

Removes unnecessary Accounts constructors #34471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a0f17f5
Removes new_with_config()
brooksprumo Dec 14, 2023
1557207
Removes default_for_tests()
brooksprumo Dec 14, 2023
4115b42
Removes new_with_config_for_benches()
brooksprumo Dec 14, 2023
e7f48d0
Removes new_with_config_for_tests()
brooksprumo Dec 14, 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
7 changes: 4 additions & 3 deletions accounts-bench/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use {
accounts::Accounts,
accounts_db::{
test_utils::{create_test_accounts, update_accounts_bench},
AccountShrinkThreshold, CalcAccountsHashDataSource,
AccountShrinkThreshold, AccountsDb, CalcAccountsHashDataSource,
},
accounts_index::AccountSecondaryIndexes,
ancestors::Ancestors,
Expand All @@ -19,7 +19,7 @@ use {
solana_sdk::{
genesis_config::ClusterType, pubkey::Pubkey, sysvar::epoch_schedule::EpochSchedule,
},
std::{env, fs, path::PathBuf},
std::{env, fs, path::PathBuf, sync::Arc},
};

fn main() {
Expand Down Expand Up @@ -69,12 +69,13 @@ fn main() {
if fs::remove_dir_all(path.clone()).is_err() {
println!("Warning: Couldn't remove {path:?}");
}
let accounts = Accounts::new_with_config_for_benches(
let accounts_db = AccountsDb::new_with_config_for_benches(
vec![path],
&ClusterType::Testnet,
AccountSecondaryIndexes::default(),
AccountShrinkThreshold::default(),
);
let accounts = Accounts::new(Arc::new(accounts_db));
Copy link
Contributor

Choose a reason for hiding this comment

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

Almost all of the cases you've touched look something like this one:

let accounts = Accounts::new(Arc::new(accounts_db))

Wondering if we should make Accounts::new() take an AccountsDb instead of Arc<AccountsDb>, and then we can shift the repeated Arc::new() inside Accounts::new()?

Also, @apfitzge had a clever suggestion on one of my PR's the other day that might be applicable here if we want to accept Arc<AccountsDb> or AccountsDb: #34449 (comment)

Copy link
Contributor Author

@brooksprumo brooksprumo Dec 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Wondering if we should make Accounts::new() take an AccountsDb instead of Arc<AccountsDb>, and then we can shift the repeated Arc::new() inside Accounts::new()?

Yeah, I'd prefer if Accounts::new() took AccountsDb instead of Arc<AccountsDb>. But, Bank::_new_from_parent() clones the AccountsDb into the new bank's Accounts, so there must be a ctor that takes an Arc<AccountsDb>.

solana/runtime/src/bank.rs

Line 1274 in aaccbdd

accounts: Arc::new(Accounts::new(accounts_db)),

We could keep a second ctor around, but I'm really trying to reduce ctors where possible.

Also, @apfitzge had a clever suggestion on one of my PR's the other day that might be applicable here if we want to accept Arc<AccountsDb> or AccountsDb: #34449 (comment)

Clever, indeed! Since Arc already has implements From<T> for Arc<T>, this allows the caller to use .into() already. This still requires the caller to do something special. I understand the Into<Arc<AccountsDb>> as a function parameter would remove the burden from the caller, which may be nice. I'm on the fence. Want me to do the Into way? I'll do basically anything to avoid adding more constructors heh.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll do basically anything to avoid adding more constructors heh.

😆 😆 😆

I'm a little on the fence as well ha and don't have too strong of an option; I just saw the repeated code so figured I'd mention it.

That being said, I think this PR is still valuable as-is; Arc::new() is pretty simple and having some of these repeated is probably better than someone having to choose from a handful of constructors. So, I think we could defer a decision to another PR and ship this one as-is

Copy link
Contributor Author

@brooksprumo brooksprumo Dec 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think I've come around to the Into<> idea. Here's a PR for it: #34476.

(I'm going to merge this PR as-is, since it feels like a complete thought, and the Into<> one is a different thought.)

println!("Creating {num_accounts} accounts");
let mut create_time = Measure::start("create accounts");
let pubkeys: Vec<_> = (0..num_slots)
Expand Down
Loading