-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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.
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
Add new flag for maximum file descriptors in rocksdb. #2386
base: master
Are you sure you want to change the base?
Changes from 5 commits
87a2240
ef6a1d0
929f0bd
09d4ddd
82eaa5f
c1bec23
9d41bd5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,6 +75,7 @@ impl BenchDb { | |
tmp_dir.path(), | ||
None, | ||
Default::default(), | ||
-1, | ||
) | ||
.unwrap(); | ||
let db = Arc::new(db); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,10 @@ use fuel_core::{ | |
combined_database::CombinedDatabase, | ||
state::historical_rocksdb::StateRewindPolicy, | ||
}; | ||
use rlimit::{ | ||
getrlimit, | ||
Resource, | ||
}; | ||
use std::path::PathBuf; | ||
|
||
/// Rollbacks the state of the blockchain to a specific block height. | ||
|
@@ -19,6 +23,17 @@ pub struct Command { | |
)] | ||
pub database_path: PathBuf, | ||
|
||
/// Defines a specific number of file descriptors that RocksDB can use. | ||
/// | ||
/// If defined as -1 no limit will be applied and will use the OS limits. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the behavior if they use -2 or any other negative value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't found anything about it in rocksdb source code in Rust or C, so I tried myself in a test and it seems to work without issues. Maybe this is the code : https://github.com/facebook/rocksdb/blob/8b38d4b4006ca9fd49432ccc16d9911919870dd2/db/db_impl/db_impl_open.cc#L57 I'm not sure that I fully understand this code but fore me if the value is -2 it will be clamped to 20. |
||
/// If not defined the system default divided by two is used. | ||
Comment on lines
+28
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reason for using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is the default value used by RocksDB for no limit that's why they are a i32. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And we want to have different behaviour between :
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, fair enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that this limit should generally be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RocksDB doesn't seems to consider these values as error : #2386 (comment) But we can do. I don't think it's very useful because it doesn't creates any problem and the only reason to tryy a -2 is to tryy to make a problem. |
||
#[clap( | ||
long = "rocksdb-max-fds", | ||
env, | ||
default_value = getrlimit(Resource::NOFILE).map(|(_, hard)| i32::try_from(hard.saturating_div(2)).unwrap_or(i32::MAX)).expect("Our supported platforms should return max FD.").to_string() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is quite long to be in an attribute. How about breaking this out to a helper function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
)] | ||
pub rocksdb_max_fds: i32, | ||
|
||
/// The path to the database. | ||
#[clap(long = "target-block-height")] | ||
pub target_block_height: u32, | ||
|
@@ -32,6 +47,7 @@ pub async fn exec(command: Command) -> anyhow::Result<()> { | |
path, | ||
64 * 1024 * 1024, | ||
StateRewindPolicy::RewindFullRange, | ||
command.rocksdb_max_fds, | ||
) | ||
.map_err(Into::<anyhow::Error>::into) | ||
.context(format!("failed to open combined database at path {path:?}"))?; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,6 +65,10 @@ use pyroscope_pprofrs::{ | |
pprof_backend, | ||
PprofConfig, | ||
}; | ||
use rlimit::{ | ||
getrlimit, | ||
Resource, | ||
}; | ||
use std::{ | ||
env, | ||
net, | ||
|
@@ -126,6 +130,19 @@ pub struct Command { | |
)] | ||
pub database_type: DbType, | ||
|
||
#[cfg(feature = "rocksdb")] | ||
|
||
/// Defines a specific number of file descriptors that RocksDB can use. | ||
/// | ||
/// If defined as -1 no limit will be applied and will use the OS limits. | ||
/// If not defined the system default divided by two is used. | ||
#[clap( | ||
long = "rocksdb-max-fds", | ||
env, | ||
default_value = getrlimit(Resource::NOFILE).map(|(_, hard)| i32::try_from(hard.saturating_div(2)).unwrap_or(i32::MAX)).expect("Our supported platforms should return max FD.").to_string() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
)] | ||
pub rocksdb_max_fds: i32, | ||
|
||
#[cfg(feature = "rocksdb")] | ||
/// Defines the state rewind policy for the database when RocksDB is enabled. | ||
/// | ||
|
@@ -273,6 +290,8 @@ impl Command { | |
database_path, | ||
database_type, | ||
#[cfg(feature = "rocksdb")] | ||
rocksdb_max_fds, | ||
#[cfg(feature = "rocksdb")] | ||
state_rewind_duration, | ||
db_prune, | ||
snapshot, | ||
|
@@ -441,6 +460,8 @@ impl Command { | |
max_database_cache_size, | ||
#[cfg(feature = "rocksdb")] | ||
state_rewind_policy, | ||
#[cfg(feature = "rocksdb")] | ||
max_fds: rocksdb_max_fds, | ||
}; | ||
|
||
let block_importer = fuel_core::service::config::fuel_core_importer::Config::new( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,10 @@ use fuel_core::{ | |
types::fuel_types::ContractId, | ||
}; | ||
use fuel_core_chain_config::ChainConfig; | ||
use rlimit::{ | ||
getrlimit, | ||
Resource, | ||
}; | ||
use std::path::{ | ||
Path, | ||
PathBuf, | ||
|
@@ -29,6 +33,17 @@ pub struct Command { | |
)] | ||
pub database_path: PathBuf, | ||
|
||
/// Defines a specific number of file descriptors that RocksDB can use. | ||
/// | ||
/// If defined as -1 no limit will be applied and will use the OS limits. | ||
/// If not defined the system default divided by two is used. | ||
#[clap( | ||
long = "rocksdb-max-fds", | ||
env, | ||
default_value = getrlimit(Resource::NOFILE).map(|(_, hard)| i32::try_from(hard.saturating_div(2)).unwrap_or(i32::MAX)).expect("Our supported platforms should return max FD.").to_string() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
)] | ||
pub rocksdb_max_fds: i32, | ||
|
||
/// Where to save the snapshot | ||
#[arg(name = "OUTPUT_DIR", long = "output-directory")] | ||
pub output_dir: PathBuf, | ||
|
@@ -125,6 +140,7 @@ pub async fn exec(command: Command) -> anyhow::Result<()> { | |
let db = open_db( | ||
&command.database_path, | ||
Some(command.max_database_cache_size), | ||
command.rocksdb_max_fds, | ||
)?; | ||
let output_dir = command.output_dir; | ||
let shutdown_listener = ShutdownListener::spawn(); | ||
|
@@ -180,11 +196,16 @@ fn load_chain_config_or_use_testnet(path: Option<&Path>) -> anyhow::Result<Chain | |
} | ||
} | ||
|
||
fn open_db(path: &Path, capacity: Option<usize>) -> anyhow::Result<CombinedDatabase> { | ||
fn open_db( | ||
path: &Path, | ||
capacity: Option<usize>, | ||
max_fds: i32, | ||
) -> anyhow::Result<CombinedDatabase> { | ||
CombinedDatabase::open( | ||
path, | ||
capacity.unwrap_or(1024 * 1024 * 1024), | ||
StateRewindPolicy::NoRewind, | ||
max_fds, | ||
) | ||
.map_err(Into::<anyhow::Error>::into) | ||
.context(format!("failed to open combined database at path {path:?}",)) | ||
|
@@ -668,7 +689,8 @@ mod tests { | |
let db_path = temp_dir.path().join("db"); | ||
std::fs::create_dir(&db_path)?; | ||
|
||
let mut db = DbPopulator::new(open_db(&db_path, None)?, StdRng::seed_from_u64(2)); | ||
let mut db = | ||
DbPopulator::new(open_db(&db_path, None, 512)?, StdRng::seed_from_u64(2)); | ||
let state = db.given_persisted_data(); | ||
db.flush(); | ||
|
||
|
@@ -681,6 +703,7 @@ mod tests { | |
chain_config: None, | ||
encoding_command: Some(EncodingCommand::Encoding { encoding }), | ||
}, | ||
rocksdb_max_fds: 512, | ||
}); | ||
|
||
// Because the test_case macro doesn't work with async tests | ||
|
@@ -720,7 +743,8 @@ mod tests { | |
|
||
let snapshot_dir = temp_dir.path().join("snapshot"); | ||
let db_path = temp_dir.path().join("db"); | ||
let mut db = DbPopulator::new(open_db(&db_path, None)?, StdRng::seed_from_u64(2)); | ||
let mut db = | ||
DbPopulator::new(open_db(&db_path, None, 512)?, StdRng::seed_from_u64(2)); | ||
|
||
let state = db.given_persisted_data(); | ||
db.flush(); | ||
|
@@ -739,6 +763,7 @@ mod tests { | |
}, | ||
}), | ||
}, | ||
rocksdb_max_fds: 512, | ||
}); | ||
|
||
tokio::runtime::Runtime::new() | ||
|
@@ -763,7 +788,8 @@ mod tests { | |
let snapshot_dir = temp_dir.path().join("snapshot"); | ||
|
||
let db_path = temp_dir.path().join("db"); | ||
let mut db = DbPopulator::new(open_db(&db_path, None)?, StdRng::seed_from_u64(2)); | ||
let mut db = | ||
DbPopulator::new(open_db(&db_path, None, 512)?, StdRng::seed_from_u64(2)); | ||
|
||
let original_state = db.given_persisted_data().sorted().into_state_config(); | ||
|
||
|
@@ -789,6 +815,7 @@ mod tests { | |
output_dir: snapshot_dir.clone(), | ||
max_database_cache_size: DEFAULT_DATABASE_CACHE_SIZE, | ||
subcommand: SubCommands::Contract { contract_id }, | ||
rocksdb_max_fds: 512, | ||
}) | ||
.await?; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few places that we're still using
-1
. Is that okay?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have let in benches because benches didn't used the test value before and I don't want them to be slowed down by a new limit (maybe it don't change anything but there is no risk for benches I think)