Skip to content

Commit e21dfe8

Browse files
authored
Internalize BevyManifest logic. Switch to RwLock (bevyengine#18263)
# Objective Fixes bevyengine#18103 bevyengine#17330 introduced a significant compile time performance regression (affects normal builds, clippy, and Rust Analyzer). While it did fix the type-resolution bug (and the general approach there is still our best known solution to the problem that doesn't involve [significant maintenance overhead](bevyengine#18103 (comment))), the changes had a couple of issues: 1. It used a Mutex, which poses a significant threat to parallelization. 2. It externalized existing, relatively simple, performance critical Bevy code to a crate outside of our control. I am not comfortable doing that for cases like this. Going forward @bevyengine/maintainer-team should be much stricter about this. 3. There were a number of other areas that introduced complexity and overhead that I consider unnecessary for our use case. On a case by case basis, if we encounter a need for more capabilities we can add them (and weigh them against the cost of doing so). ## Solution 1. I moved us back to our original code as a baseline 2. I selectively ported over the minimal changes required to fix the type resolution bug 3. I swapped `Mutex<BTreeMap<PathBuf, &'static Mutex<CargoManifest>>>` for `RwLock<BTreeMap<PathBuf, CargoManifest>>`. Note that I used the `parking_lot` RwLock because it has a mapping API that enables us to return mapped guards.
1 parent ecccd57 commit e21dfe8

File tree

6 files changed

+105
-79
lines changed

6 files changed

+105
-79
lines changed

crates/bevy_encase_derive/src/lib.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ const ENCASE: &str = "encase";
1414
fn bevy_encase_path() -> syn::Path {
1515
let bevy_manifest = BevyManifest::shared();
1616
bevy_manifest
17-
.get_subcrate("render")
17+
.maybe_get_path("bevy_render")
1818
.map(|bevy_render_path| {
1919
let mut segments = bevy_render_path.segments;
2020
segments.push(BevyManifest::parse_str("render_resource"));
@@ -31,7 +31,7 @@ fn bevy_encase_path() -> syn::Path {
3131
segments,
3232
}
3333
})
34-
.unwrap_or_else(|_err| bevy_manifest.get_path(ENCASE))
34+
.unwrap_or_else(|| bevy_manifest.get_path(ENCASE))
3535
}
3636

3737
implement!(bevy_encase_path());

crates/bevy_macro_utils/Cargo.toml

+4-1
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,10 @@ keywords = ["bevy"]
1212
syn = "2.0"
1313
quote = "1.0"
1414
proc-macro2 = "1.0"
15-
cargo-manifest-proc-macros = "0.3.4"
15+
toml_edit = { version = "0.22.7", default-features = false, features = [
16+
"parse",
17+
] }
18+
parking_lot = { version = "0.12" }
1619

1720
[lints]
1821
workspace = true
+98-49
Original file line numberDiff line numberDiff line change
@@ -1,61 +1,124 @@
11
extern crate proc_macro;
22

3-
use std::sync::MutexGuard;
4-
5-
use cargo_manifest_proc_macros::{
6-
CargoManifest, CrateReExportingPolicy, KnownReExportingCrate, PathPiece,
7-
TryResolveCratePathError,
8-
};
3+
use alloc::collections::BTreeMap;
4+
use parking_lot::{lock_api::RwLockReadGuard, MappedRwLockReadGuard, RwLock, RwLockWriteGuard};
95
use proc_macro::TokenStream;
6+
use std::{
7+
env,
8+
path::{Path, PathBuf},
9+
time::SystemTime,
10+
};
11+
use toml_edit::{DocumentMut, Item};
1012

11-
struct BevyReExportingPolicy;
12-
13-
impl CrateReExportingPolicy for BevyReExportingPolicy {
14-
fn get_re_exported_crate_path(&self, crate_name: &str) -> Option<PathPiece> {
15-
crate_name.strip_prefix("bevy_").map(|s| {
16-
let mut path = PathPiece::new();
17-
path.push(syn::parse_str::<syn::PathSegment>(s).unwrap());
18-
path
19-
})
20-
}
13+
/// The path to the `Cargo.toml` file for the Bevy project.
14+
#[derive(Debug)]
15+
pub struct BevyManifest {
16+
manifest: DocumentMut,
17+
modified_time: SystemTime,
2118
}
2219

2320
const BEVY: &str = "bevy";
2421

25-
const KNOWN_RE_EXPORTING_CRATE_BEVY: KnownReExportingCrate = KnownReExportingCrate {
26-
re_exporting_crate_package_name: BEVY,
27-
crate_re_exporting_policy: &BevyReExportingPolicy {},
28-
};
22+
impl BevyManifest {
23+
/// Returns a global shared instance of the [`BevyManifest`] struct.
24+
pub fn shared() -> MappedRwLockReadGuard<'static, BevyManifest> {
25+
static MANIFESTS: RwLock<BTreeMap<PathBuf, BevyManifest>> = RwLock::new(BTreeMap::new());
26+
let manifest_path = Self::get_manifest_path();
27+
let modified_time = Self::get_manifest_modified_time(&manifest_path)
28+
.expect("The Cargo.toml should have a modified time");
2929

30-
const ALL_KNOWN_RE_EXPORTING_CRATES: &[&KnownReExportingCrate] = &[&KNOWN_RE_EXPORTING_CRATE_BEVY];
30+
if let Ok(manifest) =
31+
RwLockReadGuard::try_map(MANIFESTS.read(), |manifests| manifests.get(&manifest_path))
32+
{
33+
if manifest.modified_time == modified_time {
34+
return manifest;
35+
}
36+
}
37+
let mut manifests = MANIFESTS.write();
38+
manifests.insert(
39+
manifest_path.clone(),
40+
BevyManifest {
41+
manifest: Self::read_manifest(&manifest_path),
42+
modified_time,
43+
},
44+
);
3145

32-
/// The path to the `Cargo.toml` file for the Bevy project.
33-
pub struct BevyManifest(MutexGuard<'static, CargoManifest>);
46+
RwLockReadGuard::map(RwLockWriteGuard::downgrade(manifests), |manifests| {
47+
manifests.get(&manifest_path).unwrap()
48+
})
49+
}
3450

35-
impl BevyManifest {
36-
/// Returns a global shared instance of the [`BevyManifest`] struct.
37-
pub fn shared() -> Self {
38-
Self(CargoManifest::shared())
51+
fn get_manifest_path() -> PathBuf {
52+
env::var_os("CARGO_MANIFEST_DIR")
53+
.map(|path| {
54+
let mut path = PathBuf::from(path);
55+
path.push("Cargo.toml");
56+
assert!(
57+
path.exists(),
58+
"Cargo manifest does not exist at path {}",
59+
path.display()
60+
);
61+
path
62+
})
63+
.expect("CARGO_MANIFEST_DIR is not defined.")
64+
}
65+
66+
fn get_manifest_modified_time(
67+
cargo_manifest_path: &Path,
68+
) -> Result<SystemTime, std::io::Error> {
69+
std::fs::metadata(cargo_manifest_path).and_then(|metadata| metadata.modified())
70+
}
71+
72+
fn read_manifest(path: &Path) -> DocumentMut {
73+
let manifest = std::fs::read_to_string(path)
74+
.unwrap_or_else(|_| panic!("Unable to read cargo manifest: {}", path.display()));
75+
manifest
76+
.parse::<DocumentMut>()
77+
.unwrap_or_else(|_| panic!("Failed to parse cargo manifest: {}", path.display()))
3978
}
4079

4180
/// Attempt to retrieve the [path](syn::Path) of a particular package in
4281
/// the [manifest](BevyManifest) by [name](str).
43-
pub fn maybe_get_path(&self, name: &str) -> Result<syn::Path, TryResolveCratePathError> {
44-
self.0
45-
.try_resolve_crate_path(name, ALL_KNOWN_RE_EXPORTING_CRATES)
46-
}
82+
pub fn maybe_get_path(&self, name: &str) -> Option<syn::Path> {
83+
let find_in_deps = |deps: &Item| -> Option<syn::Path> {
84+
let package = if deps.get(name).is_some() {
85+
return Some(Self::parse_str(name));
86+
} else if deps.get(BEVY).is_some() {
87+
BEVY
88+
} else {
89+
// Note: to support bevy crate aliases, we could do scanning here to find a crate with a "package" name that
90+
// matches our request, but that would then mean we are scanning every dependency (and dev dependency) for every
91+
// macro execution that hits this branch (which includes all built-in bevy crates). Our current stance is that supporting
92+
// remapped crate names in derive macros is not worth that "compile time" price of admission. As a workaround, people aliasing
93+
// bevy crate names can use "use REMAPPED as bevy_X" or "use REMAPPED::x as bevy_x".
94+
return None;
95+
};
4796

48-
/// Returns the path for the crate with the given name.
49-
pub fn get_path(&self, name: &str) -> syn::Path {
50-
self.maybe_get_path(name)
51-
.expect("Failed to get path for crate")
97+
let mut path = Self::parse_str::<syn::Path>(package);
98+
if let Some(module) = name.strip_prefix("bevy_") {
99+
path.segments.push(Self::parse_str(module));
100+
}
101+
Some(path)
102+
};
103+
104+
let deps = self.manifest.get("dependencies");
105+
let deps_dev = self.manifest.get("dev-dependencies");
106+
107+
deps.and_then(find_in_deps)
108+
.or_else(|| deps_dev.and_then(find_in_deps))
52109
}
53110

54111
/// Attempt to parse the provided [path](str) as a [syntax tree node](syn::parse::Parse)
55112
pub fn try_parse_str<T: syn::parse::Parse>(path: &str) -> Option<T> {
56113
syn::parse(path.parse::<TokenStream>().ok()?).ok()
57114
}
58115

116+
/// Returns the path for the crate with the given name.
117+
pub fn get_path(&self, name: &str) -> syn::Path {
118+
self.maybe_get_path(name)
119+
.unwrap_or_else(|| Self::parse_str(name))
120+
}
121+
59122
/// Attempt to parse provided [path](str) as a [syntax tree node](syn::parse::Parse).
60123
///
61124
/// # Panics
@@ -66,18 +129,4 @@ impl BevyManifest {
66129
pub fn parse_str<T: syn::parse::Parse>(path: &str) -> T {
67130
Self::try_parse_str(path).unwrap()
68131
}
69-
70-
/// Attempt to get a subcrate [path](syn::Path) under Bevy by [name](str)
71-
pub fn get_subcrate(&self, subcrate: &str) -> Result<syn::Path, TryResolveCratePathError> {
72-
self.maybe_get_path(BEVY)
73-
.map(|bevy_path| {
74-
let mut segments = bevy_path.segments;
75-
segments.push(BevyManifest::parse_str(subcrate));
76-
syn::Path {
77-
leading_colon: None,
78-
segments,
79-
}
80-
})
81-
.or_else(|_err| self.maybe_get_path(&format!("bevy_{subcrate}")))
82-
}
83132
}

crates/bevy_macro_utils/src/lib.rs

+1
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
//! A collection of helper types and functions for working on macros within the Bevy ecosystem.
99
10+
extern crate alloc;
1011
extern crate proc_macro;
1112

1213
mod attrs;

tests-integration/remapped-test/Cargo.toml

-6
This file was deleted.

tests-integration/remapped-test/src/lib.rs

-21
This file was deleted.

0 commit comments

Comments
 (0)