Skip to content

refactor: replace InternedString with Cow in IndexPackage #15559

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+36 −21
Draft

refactor: replace InternedString with Cow in IndexPackage #15559

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 36 additions & 21 deletions src/cargo/sources/registry/index/mod.rs
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could change InternString::from_cow to something like:

impl<'a> From<Cow<'a, str>> for InternedString {
    fn from(cs: Cow<'a, str>) -> Self {
        let mut cache = interned_storage();
        let s = cache.get(cs.as_ref()).copied().unwrap_or_else(|| {
            let s = cs.into_owned().leak();
            cache.insert(s);
            s
        });

        InternedString { inner: s }
    }
}

So in other places we can rely on Into<InternedString>.

Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,8 @@ impl IndexSummary {
#[derive(Deserialize, Serialize)]
pub struct IndexPackage<'a> {
/// Name of the package.
pub name: InternedString,
#[serde(borrow)]
pub name: Cow<'a, str>,
/// The version of this dependency.
pub vers: Version,
/// All kinds of direct dependencies of the package, including dev and
Expand All @@ -207,14 +208,14 @@ pub struct IndexPackage<'a> {
pub deps: Vec<RegistryDependency<'a>>,
/// Set of features defined for the package, i.e., `[features]` table.
#[serde(default)]
pub features: BTreeMap<InternedString, Vec<InternedString>>,
pub features: BTreeMap<Cow<'a, str>, Vec<Cow<'a, str>>>,
/// This field contains features with new, extended syntax. Specifically,
/// namespaced features (`dep:`) and weak dependencies (`pkg?/feat`).
///
/// This is separated from `features` because versions older than 1.19
/// will fail to load due to not being able to parse the new syntax, even
/// with a `Cargo.lock` file.
pub features2: Option<BTreeMap<InternedString, Vec<InternedString>>>,
pub features2: Option<BTreeMap<Cow<'a, str>, Vec<Cow<'a, str>>>>,
/// Checksum for verifying the integrity of the corresponding downloaded package.
pub cksum: String,
/// If `true`, Cargo will skip this version when resolving.
Expand All @@ -226,7 +227,7 @@ pub struct IndexPackage<'a> {
///
/// Added early 2018 (see <https://github.com/rust-lang/cargo/pull/4978>),
/// can be `None` if published before then.
pub links: Option<InternedString>,
pub links: Option<Cow<'a, str>>,
/// Required version of rust
///
/// Corresponds to `package.rust-version`.
Expand Down Expand Up @@ -263,7 +264,11 @@ impl IndexPackage<'_> {
fn to_summary(&self, source_id: SourceId) -> CargoResult<Summary> {
// ****CAUTION**** Please be extremely careful with returning errors, see
// `IndexSummary::parse` for details
let pkgid = PackageId::new(self.name.into(), self.vers.clone(), source_id);
let pkgid = PackageId::new(
InternedString::new(&self.name),
self.vers.clone(),
source_id,
);
let deps = self
.deps
.iter()
Expand All @@ -272,24 +277,31 @@ impl IndexPackage<'_> {
let mut features = self.features.clone();
if let Some(features2) = &self.features2 {
for (name, values) in features2 {
features.entry(*name).or_default().extend(values);
features
.entry(name.clone())
.or_default()
.extend(values.iter().cloned());
}
}
let mut summary = Summary::new(
pkgid,
deps,
&features,
self.links,
self.rust_version.clone(),
)?;
let features = features
.into_iter()
.map(|(name, values)| {
(
InternedString::new(&name),
values.iter().map(|v| InternedString::new(&v)).collect(),
)
})
.collect::<BTreeMap<_, _>>();
let links = self.links.as_ref().map(|l| InternedString::new(&l));
let mut summary = Summary::new(pkgid, deps, &features, links, self.rust_version.clone())?;
summary.set_checksum(self.cksum.clone());
Ok(summary)
}
}

#[derive(Deserialize, Serialize)]
struct IndexPackageMinimum {
name: InternedString,
struct IndexPackageMinimum<'a> {
name: Cow<'a, str>,
vers: Version,
}

Expand All @@ -308,13 +320,14 @@ struct IndexPackageV {
pub struct RegistryDependency<'a> {
/// Name of the dependency. If the dependency is renamed, the original
/// would be stored in [`RegistryDependency::package`].
pub name: InternedString,
#[serde(borrow)]
pub name: Cow<'a, str>,
/// The SemVer requirement for this dependency.
#[serde(borrow)]
pub req: Cow<'a, str>,
/// Set of features enabled for this dependency.
#[serde(default)]
pub features: Vec<InternedString>,
pub features: Vec<Cow<'a, str>>,
/// Whether or not this is an optional dependency.
#[serde(default)]
pub optional: bool,
Expand All @@ -329,7 +342,7 @@ pub struct RegistryDependency<'a> {
// `None` if it is from the same index.
pub registry: Option<Cow<'a, str>>,
/// The original name if the dependency is renamed.
pub package: Option<InternedString>,
pub package: Option<Cow<'a, str>>,
/// Whether or not this is a public dependency. Unstable. See [RFC 1977].
///
/// [RFC 1977]: https://rust-lang.github.io/rfcs/1977-public-private-dependencies.html
Expand Down Expand Up @@ -759,7 +772,7 @@ impl IndexSummary {
Ok((index, summary)) => (index, summary, true),
Err(err) => {
let Ok(IndexPackageMinimum { name, vers }) =
serde_json::from_slice::<IndexPackageMinimum>(line)
serde_json::from_slice::<IndexPackageMinimum<'_>>(line)
else {
// If we can't recover, prefer the original error
return Err(err);
Expand Down Expand Up @@ -833,9 +846,10 @@ impl<'a> RegistryDependency<'a> {
default
};

let mut dep = Dependency::parse(package.unwrap_or(name), Some(&req), id)?;
let interned_name = InternedString::new(package.as_ref().unwrap_or(&name));
let mut dep = Dependency::parse(interned_name, Some(&req), id)?;
if package.is_some() {
dep.set_explicit_name_in_toml(name);
dep.set_explicit_name_in_toml(InternedString::new(&name));
}
let kind = match kind.as_deref().unwrap_or("") {
"dev" => DepKind::Development,
Expand Down Expand Up @@ -869,6 +883,7 @@ impl<'a> RegistryDependency<'a> {
dep.set_artifact(artifact);
}

let features = features.iter().map(|f| InternedString::new(&f));
dep.set_optional(optional)
.set_default_features(default_features)
.set_features(features)
Expand Down