Skip to content

Commit acc52f3

Browse files
epageepage
committed
refactor(toml): Decouple parsing from interning system
To have a separate manifest API (#12801), we can't rely on interning because it might be used in longer-lifetime applications. I consulted https://github.com/rosetta-rs/string-rosetta-rs when deciding on what string type to use for performance. Originally, I hoped to entirely replacing string interning. For that, I was looking at `arcstr` as it had a fast equality operator. However, that is only helpful so long as the two strings we are comparing came from the original source. Unsure how likely that is to happen (and daunted by converting all of the `Copy`s into `Clone`s), I decided to scale back. Concerned about all of the small allocations when parsing a manifest, I assumed I'd need a string type with small-string optimizations, like `hipstr`, `compact_str`, `flexstr`, and `ecow`. The first three give us more wiggle room and `hipstr` was the fastest of them, so I went with that. I then double checked macro benchmarks, and realized `hipstr` made no difference and switched to `String` to keep things simple / with lower dependencies. When doing this, I had created a type alias (`TomlStr`) for the string type so I could more easily swap it out if needed (and not have to always write out a lifetime). With just using `String`, I went ahead and dropped that. I had problems getting the cargo benchmarks running, so I did a quick and dirty benchmark that is end-to-end, covering fresh builds, clean builds, and resolution. I ran these against a fresh clone of cargo's code base. ```console $ ../dump/cargo-12801-bench.rs run Finished dev [unoptimized + debuginfo] target(s) in 0.07s Running `target/debug/cargo -Zscript -Zmsrv-policy ../dump/cargo-12801-bench.rs run` warning: `package.edition` is unspecified, defaulting to `2021` Finished dev [unoptimized + debuginfo] target(s) in 0.04s Running `/home/epage/.cargo/target/0a/7f4c1ab500f045/debug/cargo-12801-bench run` $ hyperfine "../cargo-old check" "../cargo-new check" Benchmark 1: ../cargo-old check Time (mean ± σ): 119.3 ms ± 3.2 ms [User: 98.6 ms, System: 20.3 ms] Range (min … max): 115.6 ms … 124.3 ms 24 runs Benchmark 2: ../cargo-new check Time (mean ± σ): 119.4 ms ± 2.4 ms [User: 98.0 ms, System: 21.1 ms] Range (min … max): 115.7 ms … 123.6 ms 24 runs Summary ../cargo-old check ran 1.00 ± 0.03 times faster than ../cargo-new check $ hyperfine --prepare "cargo clean" "../cargo-old check" "../cargo-new check" Benchmark 1: ../cargo-old check Time (mean ± σ): 20.249 s ± 0.392 s [User: 157.719 s, System: 22.771 s] Range (min … max): 19.605 s … 21.123 s 10 runs Benchmark 2: ../cargo-new check Time (mean ± σ): 20.123 s ± 0.212 s [User: 156.156 s, System: 22.325 s] Range (min … max): 19.764 s … 20.420 s 10 runs Summary ../cargo-new check ran 1.01 ± 0.02 times faster than ../cargo-old check $ hyperfine --prepare "cargo clean && rm -f Cargo.lock" "../cargo-old check" "../cargo-new check" Benchmark 1: ../cargo-old check Time (mean ± σ): 21.105 s ± 0.465 s [User: 156.482 s, System: 22.799 s] Range (min … max): 20.156 s … 22.010 s 10 runs Benchmark 2: ../cargo-new check Time (mean ± σ): 21.358 s ± 0.538 s [User: 156.187 s, System: 22.979 s] Range (min … max): 20.703 s … 22.462 s 10 runs Summary ../cargo-old check ran 1.01 ± 0.03 times faster than ../cargo-new check ```
1 parent 708383d commit acc52f3

File tree

6 files changed

+79
-67
lines changed

6 files changed

+79
-67
lines changed

‎src/bin/cargo/commands/remove.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,7 @@ fn spec_has_match(
286286
config: &Config,
287287
) -> CargoResult<bool> {
288288
for dep in dependencies {
289-
if spec.name().as_str() != &dep.name {
289+
if spec.name() != &dep.name {
290290
continue;
291291
}
292292

‎src/cargo/core/package_id_spec.rs

Lines changed: 22 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ use url::Url;
99
use crate::core::PackageId;
1010
use crate::util::edit_distance;
1111
use crate::util::errors::CargoResult;
12-
use crate::util::interning::InternedString;
1312
use crate::util::PartialVersion;
1413
use crate::util::{validate_package_name, IntoUrl};
1514

@@ -24,7 +23,7 @@ use crate::util::{validate_package_name, IntoUrl};
2423
/// sufficient to uniquely define a package ID.
2524
#[derive(Clone, PartialEq, Eq, Debug, Hash, Ord, PartialOrd)]
2625
pub struct PackageIdSpec {
27-
name: InternedString,
26+
name: String,
2827
version: Option<PartialVersion>,
2928
url: Option<Url>,
3029
}
@@ -76,7 +75,7 @@ impl PackageIdSpec {
7675
};
7776
validate_package_name(name, "pkgid", "")?;
7877
Ok(PackageIdSpec {
79-
name: InternedString::new(name),
78+
name: String::from(name),
8079
version,
8180
url: None,
8281
})
@@ -99,7 +98,7 @@ impl PackageIdSpec {
9998
/// fields filled in.
10099
pub fn from_package_id(package_id: PackageId) -> PackageIdSpec {
101100
PackageIdSpec {
102-
name: package_id.name(),
101+
name: String::from(package_id.name().as_str()),
103102
version: Some(package_id.version().clone().into()),
104103
url: Some(package_id.source_id().url().clone()),
105104
}
@@ -127,18 +126,18 @@ impl PackageIdSpec {
127126
Some(fragment) => match fragment.split_once([':', '@']) {
128127
Some((name, part)) => {
129128
let version = part.parse::<PartialVersion>()?;
130-
(InternedString::new(name), Some(version))
129+
(String::from(name), Some(version))
131130
}
132131
None => {
133132
if fragment.chars().next().unwrap().is_alphabetic() {
134-
(InternedString::new(&fragment), None)
133+
(String::from(fragment.as_str()), None)
135134
} else {
136135
let version = fragment.parse::<PartialVersion>()?;
137-
(InternedString::new(path_name), Some(version))
136+
(String::from(path_name), Some(version))
138137
}
139138
}
140139
},
141-
None => (InternedString::new(path_name), None),
140+
None => (String::from(path_name), None),
142141
}
143142
};
144143
Ok(PackageIdSpec {
@@ -148,8 +147,8 @@ impl PackageIdSpec {
148147
})
149148
}
150149

151-
pub fn name(&self) -> InternedString {
152-
self.name
150+
pub fn name(&self) -> &str {
151+
self.name.as_str()
153152
}
154153

155154
/// Full `semver::Version`, if present
@@ -171,7 +170,7 @@ impl PackageIdSpec {
171170

172171
/// Checks whether the given `PackageId` matches the `PackageIdSpec`.
173172
pub fn matches(&self, package_id: PackageId) -> bool {
174-
if self.name() != package_id.name() {
173+
if self.name() != package_id.name().as_str() {
175174
return false;
176175
}
177176

@@ -211,7 +210,7 @@ impl PackageIdSpec {
211210
if self.url.is_some() {
212211
try_spec(
213212
PackageIdSpec {
214-
name: self.name,
213+
name: self.name.clone(),
215214
version: self.version.clone(),
216215
url: None,
217216
},
@@ -221,7 +220,7 @@ impl PackageIdSpec {
221220
if suggestion.is_empty() && self.version.is_some() {
222221
try_spec(
223222
PackageIdSpec {
224-
name: self.name,
223+
name: self.name.clone(),
225224
version: None,
226225
url: None,
227226
},
@@ -324,7 +323,6 @@ impl<'de> de::Deserialize<'de> for PackageIdSpec {
324323
mod tests {
325324
use super::PackageIdSpec;
326325
use crate::core::{PackageId, SourceId};
327-
use crate::util::interning::InternedString;
328326
use url::Url;
329327

330328
#[test]
@@ -339,7 +337,7 @@ mod tests {
339337
ok(
340338
"https://crates.io/foo",
341339
PackageIdSpec {
342-
name: InternedString::new("foo"),
340+
name: String::from("foo"),
343341
version: None,
344342
url: Some(Url::parse("https://crates.io/foo").unwrap()),
345343
},
@@ -348,7 +346,7 @@ mod tests {
348346
ok(
349347
"https://crates.io/foo#1.2.3",
350348
PackageIdSpec {
351-
name: InternedString::new("foo"),
349+
name: String::from("foo"),
352350
version: Some("1.2.3".parse().unwrap()),
353351
url: Some(Url::parse("https://crates.io/foo").unwrap()),
354352
},
@@ -357,7 +355,7 @@ mod tests {
357355
ok(
358356
"https://crates.io/foo#1.2",
359357
PackageIdSpec {
360-
name: InternedString::new("foo"),
358+
name: String::from("foo"),
361359
version: Some("1.2".parse().unwrap()),
362360
url: Some(Url::parse("https://crates.io/foo").unwrap()),
363361
},
@@ -366,7 +364,7 @@ mod tests {
366364
ok(
367365
"https://crates.io/foo#bar:1.2.3",
368366
PackageIdSpec {
369-
name: InternedString::new("bar"),
367+
name: String::from("bar"),
370368
version: Some("1.2.3".parse().unwrap()),
371369
url: Some(Url::parse("https://crates.io/foo").unwrap()),
372370
},
@@ -375,7 +373,7 @@ mod tests {
375373
ok(
376374
"https://crates.io/foo#bar@1.2.3",
377375
PackageIdSpec {
378-
name: InternedString::new("bar"),
376+
name: String::from("bar"),
379377
version: Some("1.2.3".parse().unwrap()),
380378
url: Some(Url::parse("https://crates.io/foo").unwrap()),
381379
},
@@ -384,7 +382,7 @@ mod tests {
384382
ok(
385383
"https://crates.io/foo#bar@1.2",
386384
PackageIdSpec {
387-
name: InternedString::new("bar"),
385+
name: String::from("bar"),
388386
version: Some("1.2".parse().unwrap()),
389387
url: Some(Url::parse("https://crates.io/foo").unwrap()),
390388
},
@@ -393,7 +391,7 @@ mod tests {
393391
ok(
394392
"foo",
395393
PackageIdSpec {
396-
name: InternedString::new("foo"),
394+
name: String::from("foo"),
397395
version: None,
398396
url: None,
399397
},
@@ -402,7 +400,7 @@ mod tests {
402400
ok(
403401
"foo:1.2.3",
404402
PackageIdSpec {
405-
name: InternedString::new("foo"),
403+
name: String::from("foo"),
406404
version: Some("1.2.3".parse().unwrap()),
407405
url: None,
408406
},
@@ -411,7 +409,7 @@ mod tests {
411409
ok(
412410
"foo@1.2.3",
413411
PackageIdSpec {
414-
name: InternedString::new("foo"),
412+
name: String::from("foo"),
415413
version: Some("1.2.3".parse().unwrap()),
416414
url: None,
417415
},
@@ -420,7 +418,7 @@ mod tests {
420418
ok(
421419
"foo@1.2",
422420
PackageIdSpec {
423-
name: InternedString::new("foo"),
421+
name: String::from("foo"),
424422
version: Some("1.2".parse().unwrap()),
425423
url: None,
426424
},

‎src/cargo/core/profiles.rs

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ impl Profiles {
104104
// Verify that the requested profile is defined *somewhere*.
105105
// This simplifies the API (no need for CargoResult), and enforces
106106
// assumptions about how config profiles are loaded.
107-
profile_makers.get_profile_maker(requested_profile)?;
107+
profile_makers.get_profile_maker(&requested_profile)?;
108108
Ok(profile_makers)
109109
}
110110

@@ -142,21 +142,21 @@ impl Profiles {
142142
(
143143
"bench",
144144
TomlProfile {
145-
inherits: Some(InternedString::new("release")),
145+
inherits: Some(String::from("release")),
146146
..TomlProfile::default()
147147
},
148148
),
149149
(
150150
"test",
151151
TomlProfile {
152-
inherits: Some(InternedString::new("dev")),
152+
inherits: Some(String::from("dev")),
153153
..TomlProfile::default()
154154
},
155155
),
156156
(
157157
"doc",
158158
TomlProfile {
159-
inherits: Some(InternedString::new("dev")),
159+
inherits: Some(String::from("dev")),
160160
..TomlProfile::default()
161161
},
162162
),
@@ -173,7 +173,7 @@ impl Profiles {
173173
match &profile.dir_name {
174174
None => {}
175175
Some(dir_name) => {
176-
self.dir_names.insert(name, dir_name.to_owned());
176+
self.dir_names.insert(name, InternedString::new(dir_name));
177177
}
178178
}
179179

@@ -212,12 +212,13 @@ impl Profiles {
212212
set: &mut HashSet<InternedString>,
213213
profiles: &BTreeMap<InternedString, TomlProfile>,
214214
) -> CargoResult<ProfileMaker> {
215-
let mut maker = match profile.inherits {
215+
let mut maker = match &profile.inherits {
216216
Some(inherits_name) if inherits_name == "dev" || inherits_name == "release" => {
217217
// These are the root profiles added in `add_root_profiles`.
218-
self.get_profile_maker(inherits_name).unwrap().clone()
218+
self.get_profile_maker(&inherits_name).unwrap().clone()
219219
}
220220
Some(inherits_name) => {
221+
let inherits_name = InternedString::new(&inherits_name);
221222
if !set.insert(inherits_name) {
222223
bail!(
223224
"profile inheritance loop detected with profile `{}` inheriting `{}`",
@@ -263,7 +264,7 @@ impl Profiles {
263264
unit_for: UnitFor,
264265
kind: CompileKind,
265266
) -> Profile {
266-
let maker = self.get_profile_maker(self.requested_profile).unwrap();
267+
let maker = self.get_profile_maker(&self.requested_profile).unwrap();
267268
let mut profile = maker.get_profile(Some(pkg_id), is_member, unit_for.is_for_host());
268269

269270
// Dealing with `panic=abort` and `panic=unwind` requires some special
@@ -325,7 +326,7 @@ impl Profiles {
325326
/// select for the package that was actually built.
326327
pub fn base_profile(&self) -> Profile {
327328
let profile_name = self.requested_profile;
328-
let maker = self.get_profile_maker(profile_name).unwrap();
329+
let maker = self.get_profile_maker(&profile_name).unwrap();
329330
maker.get_profile(None, /*is_member*/ true, /*is_for_host*/ false)
330331
}
331332

@@ -372,9 +373,9 @@ impl Profiles {
372373
}
373374

374375
/// Returns the profile maker for the given profile name.
375-
fn get_profile_maker(&self, name: InternedString) -> CargoResult<&ProfileMaker> {
376+
fn get_profile_maker(&self, name: &str) -> CargoResult<&ProfileMaker> {
376377
self.by_name
377-
.get(&name)
378+
.get(name)
378379
.ok_or_else(|| anyhow::format_err!("profile `{}` is not defined", name))
379380
}
380381
}
@@ -521,7 +522,7 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) {
521522
None => {}
522523
}
523524
if toml.codegen_backend.is_some() {
524-
profile.codegen_backend = toml.codegen_backend;
525+
profile.codegen_backend = toml.codegen_backend.as_ref().map(InternedString::from);
525526
}
526527
if toml.codegen_units.is_some() {
527528
profile.codegen_units = toml.codegen_units;
@@ -553,7 +554,7 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) {
553554
profile.incremental = incremental;
554555
}
555556
if let Some(flags) = &toml.rustflags {
556-
profile.rustflags = flags.clone();
557+
profile.rustflags = flags.iter().map(InternedString::from).collect();
557558
}
558559
profile.strip = match toml.strip {
559560
Some(StringOrBool::Bool(true)) => Strip::Named(InternedString::new("symbols")),
@@ -1162,7 +1163,11 @@ fn merge_config_profiles(
11621163
requested_profile: InternedString,
11631164
) -> CargoResult<BTreeMap<InternedString, TomlProfile>> {
11641165
let mut profiles = match ws.profiles() {
1165-
Some(profiles) => profiles.get_all().clone(),
1166+
Some(profiles) => profiles
1167+
.get_all()
1168+
.iter()
1169+
.map(|(k, v)| (InternedString::new(k), v.clone()))
1170+
.collect(),
11661171
None => BTreeMap::new(),
11671172
};
11681173
// Set of profile names to check if defined in config only.
@@ -1174,7 +1179,7 @@ fn merge_config_profiles(
11741179
profile.merge(&config_profile);
11751180
}
11761181
if let Some(inherits) = &profile.inherits {
1177-
check_to_add.insert(*inherits);
1182+
check_to_add.insert(InternedString::new(inherits));
11781183
}
11791184
}
11801185
// Add the built-in profiles. This is important for things like `cargo
@@ -1188,10 +1193,10 @@ fn merge_config_profiles(
11881193
while !check_to_add.is_empty() {
11891194
std::mem::swap(&mut current, &mut check_to_add);
11901195
for name in current.drain() {
1191-
if !profiles.contains_key(&name) {
1196+
if !profiles.contains_key(name.as_str()) {
11921197
if let Some(config_profile) = get_config_profile(ws, &name)? {
11931198
if let Some(inherits) = &config_profile.inherits {
1194-
check_to_add.insert(*inherits);
1199+
check_to_add.insert(InternedString::new(inherits));
11951200
}
11961201
profiles.insert(name, config_profile);
11971202
}

‎src/cargo/core/workspace.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1491,7 +1491,7 @@ impl<'cfg> Workspace<'cfg> {
14911491
// Check if `dep_name` is member of the workspace, but isn't associated with current package.
14921492
self.current_opt() != Some(member) && member.name() == *dep_name
14931493
});
1494-
if is_member && specs.iter().any(|spec| spec.name() == *dep_name) {
1494+
if is_member && specs.iter().any(|spec| spec.name() == dep_name.as_str()) {
14951495
member_specific_features
14961496
.entry(*dep_name)
14971497
.or_default()

0 commit comments

Comments
 (0)