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.

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

Revert "ShardedLmdb takes max size in bytes, not pages" #8044

Merged
merged 1 commit into from Jul 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 0 additions & 12 deletions src/rust/engine/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/rust/engine/fs/store/src/lib.rs
Expand Up @@ -910,7 +910,7 @@ mod local {
// by python, but they're not, so...
file_dbs: ShardedLmdb::new(
files_root.clone(),
1024 * 1024 * 1024 * 1024,
1024 * 1024 * 1024 * 1024 / 10,
executor.clone(),
)
.map(Arc::new),
Expand Down
1 change: 0 additions & 1 deletion src/rust/engine/sharded_lmdb/Cargo.toml
Expand Up @@ -11,6 +11,5 @@ futures = "^0.1.16"
hashing = { path = "../hashing" }
lmdb = { git = "https://github.com/pantsbuild/lmdb-rs.git", rev = "06bdfbfc6348f6804127176e561843f214fc17f8" }
log = "0.4"
page_size = "0.4"
task_executor = { path = "../task_executor" }
tempfile = "3"
23 changes: 10 additions & 13 deletions src/rust/engine/sharded_lmdb/src/lib.rs
Expand Up @@ -49,25 +49,25 @@ pub struct ShardedLmdb {
// First Database is content, second is leases.
lmdbs: HashMap<u8, (Arc<Environment>, Database, Database)>,
root_path: PathBuf,
max_size_bytes: usize,
max_size: usize,
executor: task_executor::Executor,
}

impl ShardedLmdb {
// max_size is the maximum size the databases together will be allowed to grow to in bytes.
// max_size is the maximum size the databases together will be allowed to grow to.
// When calling this function, we will attempt to allocate that much virtual (not resident) memory
// for the mmap; in theory it should be possible not to bound this, but in practice we see travis
// occasionally fail tests because it's unable to allocate virtual memory if we set this too high,
// and we have too many tests running concurrently or close together.
pub fn new(
root_path: PathBuf,
max_size_bytes: usize,
max_size: usize,
executor: task_executor::Executor,
) -> Result<ShardedLmdb, String> {
trace!("Initializing ShardedLmdb at root {:?}", root_path);
let mut lmdbs = HashMap::new();

for (env, dir, fingerprint_prefix) in ShardedLmdb::envs(&root_path, max_size_bytes)? {
for (env, dir, fingerprint_prefix) in ShardedLmdb::envs(&root_path, max_size)? {
trace!("Making ShardedLmdb content database for {:?}", dir);
let content_database = env
.create_db(Some("content"), DatabaseFlags::empty())
Expand Down Expand Up @@ -97,15 +97,12 @@ impl ShardedLmdb {
Ok(ShardedLmdb {
lmdbs,
root_path,
max_size_bytes,
max_size,
executor,
})
}

fn envs(
root_path: &Path,
max_size_bytes: usize,
) -> Result<Vec<(Environment, PathBuf, u8)>, String> {
fn envs(root_path: &Path, max_size: usize) -> Result<Vec<(Environment, PathBuf, u8)>, String> {
let mut envs = Vec::with_capacity(0x10);
for b in 0x00..0x10 {
let fingerprint_prefix = b << 4;
Expand All @@ -116,15 +113,15 @@ impl ShardedLmdb {
fs::safe_create_dir_all(&dir)
.map_err(|err| format!("Error making directory for store at {:?}: {:?}", dir, err))?;
envs.push((
ShardedLmdb::make_env(&dir, max_size_bytes)?,
ShardedLmdb::make_env(&dir, max_size)?,
dir,
fingerprint_prefix,
));
}
Ok(envs)
}

fn make_env(dir: &Path, max_size_bytes: usize) -> Result<Environment, String> {
fn make_env(dir: &Path, max_size: usize) -> Result<Environment, String> {
Environment::new()
// NO_SYNC
// =======
Expand Down Expand Up @@ -159,7 +156,7 @@ impl ShardedLmdb {
.set_flags(EnvironmentFlags::NO_SYNC | EnvironmentFlags::NO_TLS)
// 2 DBs; one for file contents, one for leases.
.set_max_dbs(2)
.set_map_size(max_size_bytes / page_size::get())
.set_map_size(max_size)
Copy link
Sponsor Member Author

@stuhood stuhood Jul 12, 2019

Choose a reason for hiding this comment

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

My read of the docs for this method is that it is supposed to be bytes, but that it should be a multiple of the page size.

https://docs.rs/lmdb/0.8.0/lmdb/struct.EnvironmentBuilder.html#method.set_map_size

.open(dir)
.map_err(|e| format!("Error making env for store at {:?}: {}", dir, e))
}
Expand Down Expand Up @@ -257,7 +254,7 @@ impl ShardedLmdb {

#[allow(clippy::identity_conversion)] // False positive: https://github.com/rust-lang/rust-clippy/issues/3913
pub fn compact(&self) -> Result<(), String> {
for (env, old_dir, _) in ShardedLmdb::envs(&self.root_path, self.max_size_bytes)? {
for (env, old_dir, _) in ShardedLmdb::envs(&self.root_path, self.max_size)? {
let new_dir = TempDir::new_in(old_dir.parent().unwrap()).expect("TODO");
env
.copy(new_dir.path(), EnvironmentCopyFlags::COMPACT)
Expand Down