Skip to content

perf(no-ticket): benchmark credential helper flows and cut custom-domain lookup overhead - #395

Draft
cloudsmith-iduffy wants to merge 4 commits into
masterfrom
perf/custom-domain-cache-hot-path
Draft

perf(no-ticket): benchmark credential helper flows and cut custom-domain lookup overhead#395
cloudsmith-iduffy wants to merge 4 commits into
masterfrom
perf/custom-domain-cache-hot-path

Conversation

@cloudsmith-iduffy

@cloudsmith-iduffy cloudsmith-iduffy commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Description

This PR sets up CodSpeed benchmarking for the credential helper flows and lands the first fixes those benchmarks surfaced.

Benchmarks and CI. benchmarks/test_credential_helpers.py measures each in-process flow a helper runs per invocation: cargo/docker/pnpm protocol handling, standard and custom domain matching, default-domain loading, and provider-chain resolution. All benchmarks run without network access; the custom-domain flow is served from a pre-warmed cache. .github/workflows/codspeed.yml runs them with CodSpeedHQ/action (pinned, simulation mode). The default pytest run does not collect them. The workflow is manual (workflow_dispatch) for now: the upload step fails with 401 until the repo is connected on codspeed.io. After the connection (plus the CODSPEED_TOKEN secret if the project is private), restore the push and pull_request triggers.

Fixes. The first benchmark run showed the custom-domain check at ~51µs per call, about 50x the standard-domain check (~1µs). Profiling attributed most of it to repeated filesystem work, all of it avoidable:

  • get_custom_domains now serves repeated lookups from a per-process memo, so a helper session reads the cache file at most once. A Cargo credential-provider session answers many requests in one process, and each request checked the same org's domains. --refresh bypasses the memo and updates it.
  • The read path no longer creates the cache directory. get_cache_path called mkdir(parents=True, exist_ok=True) on every lookup, including cache misses; only write_cache needs the directory, so only it creates the directory now.
  • is_cache_valid checks the TTL with one stat call instead of exists() + stat().

After the fixes the custom-domain flow measures ~8µs per call locally, about 6x faster. The residual cost is path resolution and the per-call host-set build.

How this was found (repro)

Run the benchmarks and read the iteration counts:

uv run pytest benchmarks/ --codspeed

Then attribute the slow flow with cProfile: warm the cache with one write_cache call, run is_cloudsmith_domain against the custom host in a loop under cProfile.Profile(), and sort by cumulative time. open() on the cache file, the double stat, and json.load dominate; the mkdir does not appear in the profile because the benchmark fixture stubs get_cache_dir — it only shows up when profiling with the real config path.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Refactoring
  • Other (please describe): performance fix plus benchmark/CI setup

Additional Notes

The memo is keyed by cache file path and lives for the process lifetime. Helper processes are short-lived, so the 7-day TTL still governs anything that outlives a single invocation. Three existing tests wrote cache files by hand and relied on get_cache_path creating the directory; they now create it themselves.

🤖 Generated with Claude Code

cloudsmith-iduffy and others added 2 commits August 26, 2026 00:12
…lows

Add pytest-codspeed benchmarks for the per-invocation helper flows:
protocol handling for cargo, docker and pnpm, domain matching for
standard and custom domains, default-domain loading, and provider-chain
resolution. A new workflow runs them on CodSpeed in simulation mode.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Serve repeated custom-domain lookups from a per-process memo, so a
helper session reads the cache file at most once. Stop creating the
cache directory on the read path; only write_cache needs it. Check the
cache TTL with one stat call instead of two.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@cloudsmith-iduffy
cloudsmith-iduffy requested a review from a team as a code owner August 25, 2026 23:13
Copilot AI lite review requested due to automatic review settings August 25, 2026 23:13

Copilot AI left a comment

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.

Pull request overview

This PR introduces CodSpeed benchmarking for the credential-helper code paths and applies performance-driven improvements to custom-domain lookup by reducing repeated filesystem work during helper sessions.

Changes:

  • Added CodSpeed benchmarks (and CI workflow) for credential helper flows.
  • Optimized custom-domain cache usage by memoizing per-process reads and moving cache-dir creation to write paths.
  • Updated tests and documentation/CHANGELOG to reflect the new caching behavior.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
uv.lock Adds the locked pytest-codspeed dependency for dev benchmarking.
pyproject.toml Adds pytest-codspeed to the dev dependency group.
cloudsmith_cli/credential_helpers/custom_domains.py Implements per-process memoization and reduces redundant filesystem operations.
cloudsmith_cli/cli/tests/commands/test_credential_helper.py Adjusts cache-dir expectations and adds tests for memo + read/write cache directory behavior.
CHANGELOG.md Notes the credential-helper custom-domain performance improvement.
benchmarks/test_credential_helpers.py New CodSpeed benchmark suite covering helper flows and domain matching.
AGENTS.md Documents how to run the new benchmark suite locally.
.github/workflows/codspeed.yml Adds a CodSpeed workflow to execute the benchmarks in CI.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines 296 to +304
cache_path = get_cache_path(org)
cached = None if refresh else read_cache(cache_path)
if cached is not None:
logger.debug("Using cached custom domains for %s", org)
return cached
if not refresh:
cached = _domains_memo.get(cache_path)
if cached is None:
cached = read_cache(cache_path)
if cached is not None:
_domains_memo[cache_path] = cached
logger.debug("Using cached custom domains for %s", org)
return list(cached)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — fixed in 8390871. The memo is now stamped with the cache file's mtime and validated by one stat call per lookup: an entry past the TTL, a rewrite by another process, or a deleted file all invalidate it, so a long-lived process (e.g. the MCP server) can no longer serve stale domains. Three tests cover the rewrite, TTL-expiry and deletion cases. The benchmark moves from ~8µs to ~11µs per call with the guard, still ~4.6x faster than the ~51µs baseline.

@cloudsmith-iduffy

Copy link
Copy Markdown
Contributor Author

Flamegraphs: custom-domain lookup, warm cache

Workload: 5,000 calls of is_cloudsmith_domain against a custom Cargo domain with a warmed cache, run under cProfile and rendered with flameprof. The same script ran on master and on this branch. Times inside the graphs include profiler overhead; use them for shape, not absolute cost. The uninstrumented benchmark numbers are ~51µs/call before and ~8µs/call after.

Before (master)

The flame is dominated by read_cache on every call: open() + json.load, the double stat in is_cache_valid, and the mkdir from get_cache_path.

before

After (this branch)

The cache file is read once, outside the hot loop. The remaining time is get_cache_path resolution and the per-call host-set build in get_format_domains.

after

The SVGs carry hover detail; open them directly for that: before · after. The images live on the assets/pr-395-flamegraphs branch; delete it after review if unwanted.

🤖 Generated with Claude Code

cloudsmith-iduffy and others added 2 commits August 26, 2026 03:10
… connected

The upload step fails with 401 until the repository is connected on
codspeed.io, so the workflow runs only on workflow_dispatch for now.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A memoized entry could outlive the 7-day TTL, a rewrite by another
process, or a deletion of the cache file. One stat call per lookup now
validates the memo, so a long-lived process cannot serve stale domains.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@cloudsmith-iduffy
cloudsmith-iduffy marked this pull request as draft August 26, 2026 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants