Skip to content

Commit

Permalink
refactor(toml): Decouple parsing from interning system
Browse files Browse the repository at this point in the history
To have a separate manifest API (rust-lang#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
```
  • Loading branch information
epage committed Oct 27, 2023
1 parent 307486e commit 905d1d5
Show file tree
Hide file tree
Showing 6 changed files with 79 additions and 67 deletions.
2 changes: 1 addition & 1 deletion src/bin/cargo/commands/remove.rs
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ fn spec_has_match(
config: &Config,
) -> CargoResult<bool> {
for dep in dependencies {
if spec.name().as_str() != &dep.name {
if spec.name() != &dep.name {
continue;
}

Expand Down
46 changes: 22 additions & 24 deletions src/cargo/core/package_id_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use url::Url;
use crate::core::PackageId;
use crate::util::edit_distance;
use crate::util::errors::CargoResult;
use crate::util::interning::InternedString;
use crate::util::PartialVersion;
use crate::util::{validate_package_name, IntoUrl};

Expand All @@ -24,7 +23,7 @@ use crate::util::{validate_package_name, IntoUrl};
/// sufficient to uniquely define a package ID.
#[derive(Clone, PartialEq, Eq, Debug, Hash, Ord, PartialOrd)]
pub struct PackageIdSpec {
name: InternedString,
name: String,
version: Option<PartialVersion>,
url: Option<Url>,
}
Expand Down Expand Up @@ -76,7 +75,7 @@ impl PackageIdSpec {
};
validate_package_name(name, "pkgid", "")?;
Ok(PackageIdSpec {
name: InternedString::new(name),
name: String::from(name),
version,
url: None,
})
Expand All @@ -99,7 +98,7 @@ impl PackageIdSpec {
/// fields filled in.
pub fn from_package_id(package_id: PackageId) -> PackageIdSpec {
PackageIdSpec {
name: package_id.name(),
name: String::from(package_id.name().as_str()),
version: Some(package_id.version().clone().into()),
url: Some(package_id.source_id().url().clone()),
}
Expand Down Expand Up @@ -127,18 +126,18 @@ impl PackageIdSpec {
Some(fragment) => match fragment.split_once([':', '@']) {
Some((name, part)) => {
let version = part.parse::<PartialVersion>()?;
(InternedString::new(name), Some(version))
(String::from(name), Some(version))
}
None => {
if fragment.chars().next().unwrap().is_alphabetic() {
(InternedString::new(&fragment), None)
(String::from(fragment.as_str()), None)
} else {
let version = fragment.parse::<PartialVersion>()?;
(InternedString::new(path_name), Some(version))
(String::from(path_name), Some(version))
}
}
},
None => (InternedString::new(path_name), None),
None => (String::from(path_name), None),
}
};
Ok(PackageIdSpec {
Expand All @@ -148,8 +147,8 @@ impl PackageIdSpec {
})
}

pub fn name(&self) -> InternedString {
self.name
pub fn name(&self) -> &str {
self.name.as_str()
}

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

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

Expand Down Expand Up @@ -211,7 +210,7 @@ impl PackageIdSpec {
if self.url.is_some() {
try_spec(
PackageIdSpec {
name: self.name,
name: self.name.clone(),
version: self.version.clone(),
url: None,
},
Expand All @@ -221,7 +220,7 @@ impl PackageIdSpec {
if suggestion.is_empty() && self.version.is_some() {
try_spec(
PackageIdSpec {
name: self.name,
name: self.name.clone(),
version: None,
url: None,
},
Expand Down Expand Up @@ -324,7 +323,6 @@ impl<'de> de::Deserialize<'de> for PackageIdSpec {
mod tests {
use super::PackageIdSpec;
use crate::core::{PackageId, SourceId};
use crate::util::interning::InternedString;
use url::Url;

#[test]
Expand All @@ -339,7 +337,7 @@ mod tests {
ok(
"https://crates.io/foo",
PackageIdSpec {
name: InternedString::new("foo"),
name: String::from("foo"),
version: None,
url: Some(Url::parse("https://crates.io/foo").unwrap()),
},
Expand All @@ -348,7 +346,7 @@ mod tests {
ok(
"https://crates.io/foo#1.2.3",
PackageIdSpec {
name: InternedString::new("foo"),
name: String::from("foo"),
version: Some("1.2.3".parse().unwrap()),
url: Some(Url::parse("https://crates.io/foo").unwrap()),
},
Expand All @@ -357,7 +355,7 @@ mod tests {
ok(
"https://crates.io/foo#1.2",
PackageIdSpec {
name: InternedString::new("foo"),
name: String::from("foo"),
version: Some("1.2".parse().unwrap()),
url: Some(Url::parse("https://crates.io/foo").unwrap()),
},
Expand All @@ -366,7 +364,7 @@ mod tests {
ok(
"https://crates.io/foo#bar:1.2.3",
PackageIdSpec {
name: InternedString::new("bar"),
name: String::from("bar"),
version: Some("1.2.3".parse().unwrap()),
url: Some(Url::parse("https://crates.io/foo").unwrap()),
},
Expand All @@ -375,7 +373,7 @@ mod tests {
ok(
"https://crates.io/foo#[email protected]",
PackageIdSpec {
name: InternedString::new("bar"),
name: String::from("bar"),
version: Some("1.2.3".parse().unwrap()),
url: Some(Url::parse("https://crates.io/foo").unwrap()),
},
Expand All @@ -384,7 +382,7 @@ mod tests {
ok(
"https://crates.io/foo#[email protected]",
PackageIdSpec {
name: InternedString::new("bar"),
name: String::from("bar"),
version: Some("1.2".parse().unwrap()),
url: Some(Url::parse("https://crates.io/foo").unwrap()),
},
Expand All @@ -393,7 +391,7 @@ mod tests {
ok(
"foo",
PackageIdSpec {
name: InternedString::new("foo"),
name: String::from("foo"),
version: None,
url: None,
},
Expand All @@ -402,7 +400,7 @@ mod tests {
ok(
"foo:1.2.3",
PackageIdSpec {
name: InternedString::new("foo"),
name: String::from("foo"),
version: Some("1.2.3".parse().unwrap()),
url: None,
},
Expand All @@ -411,7 +409,7 @@ mod tests {
ok(
"[email protected]",
PackageIdSpec {
name: InternedString::new("foo"),
name: String::from("foo"),
version: Some("1.2.3".parse().unwrap()),
url: None,
},
Expand All @@ -420,7 +418,7 @@ mod tests {
ok(
"[email protected]",
PackageIdSpec {
name: InternedString::new("foo"),
name: String::from("foo"),
version: Some("1.2".parse().unwrap()),
url: None,
},
Expand Down
39 changes: 22 additions & 17 deletions src/cargo/core/profiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ impl Profiles {
// Verify that the requested profile is defined *somewhere*.
// This simplifies the API (no need for CargoResult), and enforces
// assumptions about how config profiles are loaded.
profile_makers.get_profile_maker(requested_profile)?;
profile_makers.get_profile_maker(&requested_profile)?;
Ok(profile_makers)
}

Expand Down Expand Up @@ -142,21 +142,21 @@ impl Profiles {
(
"bench",
TomlProfile {
inherits: Some(InternedString::new("release")),
inherits: Some(String::from("release")),
..TomlProfile::default()
},
),
(
"test",
TomlProfile {
inherits: Some(InternedString::new("dev")),
inherits: Some(String::from("dev")),
..TomlProfile::default()
},
),
(
"doc",
TomlProfile {
inherits: Some(InternedString::new("dev")),
inherits: Some(String::from("dev")),
..TomlProfile::default()
},
),
Expand All @@ -173,7 +173,7 @@ impl Profiles {
match &profile.dir_name {
None => {}
Some(dir_name) => {
self.dir_names.insert(name, dir_name.to_owned());
self.dir_names.insert(name, InternedString::new(dir_name));
}
}

Expand Down Expand Up @@ -212,12 +212,13 @@ impl Profiles {
set: &mut HashSet<InternedString>,
profiles: &BTreeMap<InternedString, TomlProfile>,
) -> CargoResult<ProfileMaker> {
let mut maker = match profile.inherits {
let mut maker = match &profile.inherits {
Some(inherits_name) if inherits_name == "dev" || inherits_name == "release" => {
// These are the root profiles added in `add_root_profiles`.
self.get_profile_maker(inherits_name).unwrap().clone()
self.get_profile_maker(&inherits_name).unwrap().clone()
}
Some(inherits_name) => {
let inherits_name = InternedString::new(&inherits_name);
if !set.insert(inherits_name) {
bail!(
"profile inheritance loop detected with profile `{}` inheriting `{}`",
Expand Down Expand Up @@ -263,7 +264,7 @@ impl Profiles {
unit_for: UnitFor,
kind: CompileKind,
) -> Profile {
let maker = self.get_profile_maker(self.requested_profile).unwrap();
let maker = self.get_profile_maker(&self.requested_profile).unwrap();
let mut profile = maker.get_profile(Some(pkg_id), is_member, unit_for.is_for_host());

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

Expand Down Expand Up @@ -372,9 +373,9 @@ impl Profiles {
}

/// Returns the profile maker for the given profile name.
fn get_profile_maker(&self, name: InternedString) -> CargoResult<&ProfileMaker> {
fn get_profile_maker(&self, name: &str) -> CargoResult<&ProfileMaker> {
self.by_name
.get(&name)
.get(name)
.ok_or_else(|| anyhow::format_err!("profile `{}` is not defined", name))
}
}
Expand Down Expand Up @@ -521,7 +522,7 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) {
None => {}
}
if toml.codegen_backend.is_some() {
profile.codegen_backend = toml.codegen_backend;
profile.codegen_backend = toml.codegen_backend.as_ref().map(InternedString::from);
}
if toml.codegen_units.is_some() {
profile.codegen_units = toml.codegen_units;
Expand Down Expand Up @@ -553,7 +554,7 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) {
profile.incremental = incremental;
}
if let Some(flags) = &toml.rustflags {
profile.rustflags = flags.clone();
profile.rustflags = flags.iter().map(InternedString::from).collect();
}
profile.strip = match toml.strip {
Some(StringOrBool::Bool(true)) => Strip::Named(InternedString::new("symbols")),
Expand Down Expand Up @@ -1162,7 +1163,11 @@ fn merge_config_profiles(
requested_profile: InternedString,
) -> CargoResult<BTreeMap<InternedString, TomlProfile>> {
let mut profiles = match ws.profiles() {
Some(profiles) => profiles.get_all().clone(),
Some(profiles) => profiles
.get_all()
.iter()
.map(|(k, v)| (InternedString::new(k), v.clone()))
.collect(),
None => BTreeMap::new(),
};
// Set of profile names to check if defined in config only.
Expand All @@ -1174,7 +1179,7 @@ fn merge_config_profiles(
profile.merge(&config_profile);
}
if let Some(inherits) = &profile.inherits {
check_to_add.insert(*inherits);
check_to_add.insert(InternedString::new(inherits));
}
}
// Add the built-in profiles. This is important for things like `cargo
Expand All @@ -1188,10 +1193,10 @@ fn merge_config_profiles(
while !check_to_add.is_empty() {
std::mem::swap(&mut current, &mut check_to_add);
for name in current.drain() {
if !profiles.contains_key(&name) {
if !profiles.contains_key(name.as_str()) {
if let Some(config_profile) = get_config_profile(ws, &name)? {
if let Some(inherits) = &config_profile.inherits {
check_to_add.insert(*inherits);
check_to_add.insert(InternedString::new(inherits));
}
profiles.insert(name, config_profile);
}
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1491,7 +1491,7 @@ impl<'cfg> Workspace<'cfg> {
// Check if `dep_name` is member of the workspace, but isn't associated with current package.
self.current_opt() != Some(member) && member.name() == *dep_name
});
if is_member && specs.iter().any(|spec| spec.name() == *dep_name) {
if is_member && specs.iter().any(|spec| spec.name() == dep_name.as_str()) {
member_specific_features
.entry(*dep_name)
.or_default()
Expand Down
Loading

0 comments on commit 905d1d5

Please sign in to comment.