Switch to ImDocument
in BevyManifest
(#18272)
# Objective When reviewing #18263, I noticed that `BevyManifest` internally stores a `DocumentMut`, a mutable TOML document, instead of an `ImDocument`, an immutable one. The process of creating a `DocumentMut` first involves creating a `ImDocument` and then cloning all the referenced spans of text into their own allocations (internally referred to as `despan` in `toml_edit`). As such, using a `DocumentMut` without mutation is strictly additional overhead. In addition, I noticed that the filesystem operations associated with reading a manifest and parsing it were written to be completed _while_ a write-lock was held on `MANIFESTS`. This likely doesn't translate into a performance or deadlock issue as the manifest files are generally small and can be read quickly, but it is generally considered a bad practice. ## Solution - Switched to `ImDocument<Box<str>>` instead of `DocumentMut` - Re-ordered operations in `BevyManifest::shared` to minimise time spent holding open the write-lock on `MANIFESTS` ## Testing - CI --- ## Notes I wasn't able to measure a meaningful performance difference with this PR, so this is purely a code quality change and not one for performance. --------- Co-authored-by: Carter Anderson <mcanders1@gmail.com>
This commit is contained in:
parent
81c0900cf2
commit
dc7cb2a3e1
@ -8,12 +8,12 @@ use std::{
|
|||||||
path::{Path, PathBuf},
|
path::{Path, PathBuf},
|
||||||
time::SystemTime,
|
time::SystemTime,
|
||||||
};
|
};
|
||||||
use toml_edit::{DocumentMut, Item};
|
use toml_edit::{ImDocument, Item};
|
||||||
|
|
||||||
/// The path to the `Cargo.toml` file for the Bevy project.
|
/// The path to the `Cargo.toml` file for the Bevy project.
|
||||||
#[derive(Debug)]
|
#[derive(Debug)]
|
||||||
pub struct BevyManifest {
|
pub struct BevyManifest {
|
||||||
manifest: DocumentMut,
|
manifest: ImDocument<Box<str>>,
|
||||||
modified_time: SystemTime,
|
modified_time: SystemTime,
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -34,14 +34,15 @@ impl BevyManifest {
|
|||||||
return manifest;
|
return manifest;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
let manifest = BevyManifest {
|
||||||
|
manifest: Self::read_manifest(&manifest_path),
|
||||||
|
modified_time,
|
||||||
|
};
|
||||||
|
|
||||||
|
let key = manifest_path.clone();
|
||||||
let mut manifests = MANIFESTS.write();
|
let mut manifests = MANIFESTS.write();
|
||||||
manifests.insert(
|
manifests.insert(key, manifest);
|
||||||
manifest_path.clone(),
|
|
||||||
BevyManifest {
|
|
||||||
manifest: Self::read_manifest(&manifest_path),
|
|
||||||
modified_time,
|
|
||||||
},
|
|
||||||
);
|
|
||||||
|
|
||||||
RwLockReadGuard::map(RwLockWriteGuard::downgrade(manifests), |manifests| {
|
RwLockReadGuard::map(RwLockWriteGuard::downgrade(manifests), |manifests| {
|
||||||
manifests.get(&manifest_path).unwrap()
|
manifests.get(&manifest_path).unwrap()
|
||||||
@ -69,11 +70,11 @@ impl BevyManifest {
|
|||||||
std::fs::metadata(cargo_manifest_path).and_then(|metadata| metadata.modified())
|
std::fs::metadata(cargo_manifest_path).and_then(|metadata| metadata.modified())
|
||||||
}
|
}
|
||||||
|
|
||||||
fn read_manifest(path: &Path) -> DocumentMut {
|
fn read_manifest(path: &Path) -> ImDocument<Box<str>> {
|
||||||
let manifest = std::fs::read_to_string(path)
|
let manifest = std::fs::read_to_string(path)
|
||||||
.unwrap_or_else(|_| panic!("Unable to read cargo manifest: {}", path.display()));
|
.unwrap_or_else(|_| panic!("Unable to read cargo manifest: {}", path.display()))
|
||||||
manifest
|
.into_boxed_str();
|
||||||
.parse::<DocumentMut>()
|
ImDocument::parse(manifest)
|
||||||
.unwrap_or_else(|_| panic!("Failed to parse cargo manifest: {}", path.display()))
|
.unwrap_or_else(|_| panic!("Failed to parse cargo manifest: {}", path.display()))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
Loading…
Reference in New Issue
Block a user