Skip to content

perf(pm): drop redundant version_list / dist_tags clones on resolve hot path #2877

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

Closed
elrrrrrrr wants to merge 1 commit into from
Closed

perf(pm): drop redundant version_list / dist_tags clones on resolve hot path #2877

Show file tree
Hide file tree
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
3 changes: 1 addition & 2 deletions crates/pm/src/cmd/view.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,9 @@ pub async fn view(package_spec: &str) -> Result<()> {
tracing::debug!("Fetched package info: {full_manifest:?}");

// Resolve version and get full VersionManifest (with all display fields)
let version_list: Vec<String> = full_manifest.versions.clone();
let resolved_version = utoo_ruborist::resolver::version::resolve_target_version(
&full_manifest.dist_tags,
&version_list,
&full_manifest.versions,
version_spec,
)
.map_err(|e| {
Expand Down
6 changes: 2 additions & 4 deletions crates/ruborist/src/resolver/registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,14 +137,12 @@ pub fn resolve_from_manifest<E: std::error::Error + 'static>(
manifest: &FullManifest,
spec: &str,
) -> Result<ResolvedPackage, ResolveError<E>> {
let version_list: Vec<String> = manifest.versions.clone();

if version_list.is_empty() {
if manifest.versions.is_empty() {

@gemini-code-assist gemini-code-assist Bot Apr 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

This check (and the subsequent resolve_target_version call) will incorrectly fail with NoVersions if the manifest was obtained from a UnifiedRegistry cache. The mem::take optimization introduced in service/registry.rs empties the versions field of the cached manifest, breaking compatibility with this public function and any other external consumers of the FullManifest struct.

return Err(ResolveError::NoVersions(manifest.name.clone()));
}

// Resolve version using shared logic
let resolved_version = resolve_target_version(&manifest.dist_tags, &version_list, spec)
let resolved_version = resolve_target_version(&manifest.dist_tags, &manifest.versions, spec)
.map_err(|e| ResolveError::Version(format!("{}@{}: {}", manifest.name, spec, e)))?;

// Get manifest for resolved version (lazy: parse from Value on demand)
Expand Down
41 changes: 31 additions & 10 deletions crates/ruborist/src/service/registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,11 +244,19 @@ impl UnifiedRegistry {
.await
.map_err(RegistryError)?
{
manifest::FetchManifestResult::Ok(manifest, new_etag) => {
manifest::FetchManifestResult::Ok(mut manifest, new_etag) => {
// MOVE version_list / dist_tags out of `manifest`
// into `VersionsInfo` instead of cloning. The cached
// `Arc<FullManifest>` keeps `raw` + scalar metadata;
// canonical version_list / dist_tags now live in the
// `versions_info` cache slot only. Readers of these
// fields go through `cache.get_versions(name)`.
let version_list = std::mem::take(&mut manifest.versions);
let dist_tags = std::mem::take(&mut manifest.dist_tags);
Comment on lines +254 to +255

@gemini-code-assist gemini-code-assist Bot Apr 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Using std::mem::take to move versions and dist_tags out of the FullManifest breaks the integrity of the cached Arc<FullManifest>. While internal methods like get_core_version work by parsing the raw bytes, any code accessing the versions or dist_tags fields directly on the manifest returned by fetch_full_manifest will now receive empty data. This is a breaking change for the public API and violates the expected behavior of the FullManifest struct.

Additionally, since UnifiedRegistry does not override the default implementation of fetch_versions_info in the RegistryClient trait (which calls fetch_full_manifest), that method is now also broken and will return empty lists when serving from cache.

let versions_info = Arc::new(VersionsInfo {
versions: Versions {
version_list: manifest.versions.clone(),
dist_tags: manifest.dist_tags.clone(),
version_list,
dist_tags,
},
etag: new_etag,
last_updated: current_timestamp_secs(),
Expand All @@ -269,18 +277,20 @@ impl UnifiedRegistry {
self.cache.set_versions(name.to_string(), versions_info);
} else {
// Persistent store corrupted/missing, fetch fresh (without etag).
let (manifest, new_etag) = manifest::fetch_full_manifest_fresh(
let (mut manifest, new_etag) = manifest::fetch_full_manifest_fresh(
&self.registry_url,
name,
manifest::MetadataFormat::Abbreviated,
)
.await
.map_err(RegistryError)?;

let version_list = std::mem::take(&mut manifest.versions);
let dist_tags = std::mem::take(&mut manifest.dist_tags);
let versions_info = Arc::new(VersionsInfo {
versions: Versions {
version_list: manifest.versions.clone(),
dist_tags: manifest.dist_tags.clone(),
version_list,
dist_tags,
},
etag: new_etag,
last_updated: current_timestamp_secs(),
Expand Down Expand Up @@ -429,12 +439,23 @@ impl UnifiedRegistry {
) -> Result<(String, CoreVersionManifest), RegistryError> {
match self.resolve_full_manifest(name).await? {
FullManifestResult::Full(full) => {
if full.versions.is_empty() {
// Canonical `version_list` / `dist_tags` live in the
// `versions_info` cache slot — the 200 path moved them
// there with `std::mem::take`. Read through the cache
// instead of the (now-empty) fields on the cached
// `Arc<FullManifest>`.
let versions_info = self.cache.get_versions(name).ok_or_else(|| {
RegistryError(anyhow!("Versions cache not found for {}", name))
})?;
Comment on lines +447 to +449

@gemini-code-assist gemini-code-assist Bot Apr 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

This introduces an additional DashMap lookup (involving hashing and shard locking) on the resolution hot path. Previously, this data was available directly on the FullManifest fields. While this lookup is required by the new architecture where fields are moved out of the manifest, it partially offsets the performance gains from avoiding clones by adding sequential overhead to the aggregator task.

if versions_info.versions.version_list.is_empty() {
return Err(RegistryError(anyhow!("No versions available for {}", name)));
}
let resolved_version =
resolve_target_version(&full.dist_tags, &full.versions, spec)
.map_err(|e| RegistryError(anyhow!("{}@{}: {}", name, spec, e)))?;
let resolved_version = resolve_target_version(
&versions_info.versions.dist_tags,
&versions_info.versions.version_list,
spec,
)
.map_err(|e| RegistryError(anyhow!("{}@{}: {}", name, spec, e)))?;
let core = full.get_core_version(&resolved_version).ok_or_else(|| {
RegistryError(anyhow!(
"Version {} not found in manifest for {}",
Expand Down
18 changes: 10 additions & 8 deletions crates/ruborist/src/traits/registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,11 @@ pub trait RegistryClient {
) -> impl Future<Output = Result<Arc<CoreVersionManifest>, Self::Error>> {
async move {
let manifest = self.fetch_full_manifest(name).await?;
let version_list: Vec<String> = manifest.versions.clone();

// Resolve version using shared logic
let resolved_version = resolve_target_version(&manifest.dist_tags, &version_list, spec)
.map_err(|e| RegistryError(anyhow::anyhow!("{}@{}: {}", name, spec, e)))?;
let resolved_version =
resolve_target_version(&manifest.dist_tags, &manifest.versions, spec)
.map_err(|e| RegistryError(anyhow::anyhow!("{}@{}: {}", name, spec, e)))?;

manifest
.get_core_version(&resolved_version)
Expand Down Expand Up @@ -227,19 +227,21 @@ pub trait RegistryClient {
fetch_spec
);
let full_manifest = self.fetch_full_manifest(&fetch_name).await?;
let version_list: Vec<String> = full_manifest.versions.clone();

if version_list.is_empty() {
if full_manifest.versions.is_empty() {
return Err(RegistryError(anyhow::anyhow!(
"No versions available for {}",
fetch_name
))
.into());
}

let resolved_version =
resolve_target_version(&full_manifest.dist_tags, &version_list, &fetch_spec)
.map_err(|e| RegistryError(anyhow::anyhow!("{}@{}: {}", name, spec, e)))?;
let resolved_version = resolve_target_version(
&full_manifest.dist_tags,
&full_manifest.versions,
&fetch_spec,
)
.map_err(|e| RegistryError(anyhow::anyhow!("{}@{}: {}", name, spec, e)))?;

let version_manifest = full_manifest
.get_core_version(&resolved_version)
Expand Down
Loading