Skip to content

Speed up project-patch and purl checks (#1607) - #1701

Merged
Mikola Lysenko (mikolalysenko) merged 4 commits into
mainfrom
agent/fix-yarn-berry-rescan-residual
Oct 24, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 4 commits into
mainfrom
agent/fix-yarn-berry-rescan-residual

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

LLM Description written by Claude Code:claude-opus-5-5

Refs #1607

This speeds up two spots on every scan's path, the project-patch check and the purl key check, and cuts yarn-berry/rescan instructions by 2.7%. Most of #1607's leftover wall time and peak RSS turns out to come from the binary's growth, not from a hotspot (see What's left). So this PR uses Refs, not Fixes.

Root cause

Callgrind of yarn-berry/rescan, base cc938aa6 vs main 3114a28: 1,047M → 1,103M instructions (+56M). The largest new cost is the project-patch check (UserPatchIndex::discover, ~15.5M per call). It didn't exist at cc938aa6, and it runs twice per hosted scan: once in run_scan and once more under the write lock in run_redirect_selected_untracked. Inside it:

  • the yarn.lock scan split every line (~59k lines, 12.4M instructions per call) to find the few resolution: lines with a @patch: locator
  • the two patch-package regexes were compiled on every call (~2.4M per call), even for projects with no patch-package script

canonical_base_purl's "already canonical" fast path (#1693) also checked for ?/#/% with a per-char decode. It runs ~25k times per scan, about 700 instructions each.

Changes

  • user_patches.rs: a new lines_containing(text, "@patch:") searches for the needle and widens each hit to its line, instead of walking every line. Its output matches str::lines().filter(..): a \r before \n is dropped, a final bare \r is kept, and each line comes back once even with several hits. The loop body is unchanged, and it already required @patch: in the value, so it sees the same lines.
  • user_patches.rs: the patch-package --patch-dir and cd regexes are LazyLock statics. They compile once, on first use, only when a script mentions patch-package.
  • purl_key.rs: is_canonical_base scans bytes for ?/#/%. All three are ASCII, and no multi-byte UTF-8 sequence contains an ASCII byte, so the answer is the same.

The second project-patch check under the write lock is deliberate: it re-reads declarations so a patch added during API selection isn't lost. I kept it. It is now cheap.

Evidence

Scan --json output on the bench fixture is identical before and after, apart from durationMs.

Callgrind, yarn-berry/rescan: base 1,047M, main 1,103M, this PR 1,073M.

socket-patch-bench compare, 25 interleaved pairs (+10 confirm), 4 vCPU sandbox:

base → head scenario base wall head wall Δ wall [95% CI] Δ CPU Δ peak RSS
main 3114a28 → this PR yarn-berry/rescan 186.1 ms 179.3 ms -3.4% [-9.5, +6.4] -1.2% -0.7%
main 3114a28 → this PR yarn-berry/hosted 223.6 ms 222.2 ms +1.2% [-2.6, +3.3] -0.7% -0.3%
cc938aa6 → main yarn-berry/rescan 160.0 ms 177.9 ms +12.4% [+9.0, +15.3] +8.8% +10.4%
cc938aa6 → this PR yarn-berry/rescan 157.5 ms 171.6 ms +10.4% [+4.9, +16.4] +6.6% +10.9%

The wall gain against main is within noise. The instruction cut is real but small next to the remaining gap.

What's left (why #1607 stays open)

The remaining gap is not a hotspot:

  • Heap is unchanged. DHAT peak heap: base 14.44 MB, main 14.52 MB (+0.6%). Total allocated +8%.
  • The binary grew. The perf binary went from 44.8 MB to 50.0 MB (+12%). Minor page faults per run went from 6,459 (base) to 7,100 (main) and 7,087 (this PR): +640 faults, about 2.5 MB more pages touched. That matches the +10% peak RSS.
  • Syscalls are flat. strace -c: openat, statx and read are within 1–2% of base. Only 4 more threads.
  • What's left of the instruction growth is spread out: more HashMap inserts and rehashes (+15k inserts) and sip hashing, from features added since cc938aa6.

A deliberate binary-size pass, or re-baselining the bench, is a maintainer call. I've noted it on #1607.

Tests

  • patch::user_patches::tests::lines_containing_matches_a_filtered_lines_walk: equivalence with lines().filter(..) over edge cases (CRLF, bare \r, several hits per line, no trailing newline, non-ASCII), plus the documented contract pinned with literal expected values
  • patch::user_patches::tests::yarn_lock_patches_are_found_among_many_unpatched_entries: a CRLF lock with 200 plain entries, where the user patches at the start and end are found and a builtin compat patch is not
  • utils::purl_key::tests::canonical_fast_path_matches_the_rebuild: now also covers non-ASCII names, with and without % and ?

Local runs:

  • cargo fmt --all -- --check: clean
  • cargo clippy --workspace --all-features -- -D warnings: clean
  • cargo test --workspace --all-features --no-fail-fast: 14,873 passed, 11 failed. All 11 are environmental, and none touches the changed code:
    • 10 write-failure and unremovable-file tests, because this sandbox runs as root and root ignores the read-only permissions they set up
    • e2e_bun_lockb::binary_shared_bundled_record_hosted_pin_is_managed, which needs patches-api.socket.dev and this sandbox has no network route to it

No npm/pypi/gem wrapper changes: the wrappers only dispatch to the binary.

/code-review high

No correctness findings. Fixed:

  • lines_containing could stall on an empty or multi-line needle. It now moves past each line it returns, with a debug_assert on the needle.
  • the equivalence test only matched the toolchain's lines. It now also pins literal expected values.

Not changed:

  • "the second discover is a duplicate": it is the deliberate re-read under the write lock (see above)
  • "replace the iterator with lines().filter": that is the slow path this PR removes. The helper is 15 lines and has its own equivalence test.
  • "trim() is still Unicode-aware": it only looks at the two ends of the string, which doesn't show up in the profile
  • "empty start commit": the agent claim protocol requires it, and squash-merge drops it

https://claude.ai/code/session_01CCk3YDBWYBfufBX3DuwRTs


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
Every scan checks the project for its own patches (Yarn patch:
locators, patch-package scripts) before touching a package, and
hosted scans check again under the write lock. On a large Yarn Berry
lock that check split every line of yarn.lock to find the few
resolution lines that carry a patch: locator, and it recompiled the
two patch-package regexes each time.

Now it jumps straight to the "@patch:" occurrences and compiles the
regexes once, only for a project whose scripts run patch-package.
What it finds is unchanged. On the yarn-berry/rescan bench fixture
this cuts about 20M instructions (~2%) from each scan.

Refs #1607

Assisted-by: Claude Code:claude-opus-5-5
Scans key every crawled package's purl several times, and each key
first checks whether the purl is already canonical. That check
looked for '?', '#' and '%' by decoding every character. Scanning
bytes gives the same answer (all three are ASCII, and no multi-byte
UTF-8 character contains an ASCII byte) for a fraction of the cost:
about 10M fewer instructions per yarn-berry/rescan bench run.

Refs #1607

Assisted-by: Claude Code:claude-opus-5-5
The helper that finds yarn.lock lines holding a patch: locator now
moves past each line it returns, so a needle that is empty or holds
a newline can't keep matching the same spot. A test pins the
documented line-ending rules instead of only comparing against
str::lines.

Refs #1607

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko Mikola Lysenko (mikolalysenko) changed the title Fix yarn-berry rescan slowdown (#1607) Speed up project-patch and purl checks (#1607) Oct 23, 2026
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 23, 2026 22:58
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit d9b8faa. Configure here.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 24, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Ready for review (burn-down agent). Head d9b8faa: CI 87/87 settled green (37 success, 50 skipped). Bugbot reviewed d9b8faa: no findings. No merge conflicts, no CHANGELOG change. Reviewers: lines_containing in user_patches.rs must match str::lines() semantics; the PR is Refs #1607 (binary growth remains).


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Labeled Ready for review (burn-down agent).

  • Head: d9b8faadf622894de5603ea99a32e62aebfcf4ba, mergeable into main with no conflicts.
  • CI: 37/37 non-skipped checks green (50 skipped). No main-wide failures.
  • Bugbot: reviewed this head, no findings; no open review threads.
  • Reviewer note: lines_containing must match str::lines().filter(..) on CRLF and trailing \r; the second project-patch check under the write lock is kept on purpose.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does: It speeds up two checks that run on every scan. The yarn.lock patch scan now jumps straight to the lines that contain @patch: instead of splitting all ~59k lines. The two patch-package regexes are compiled once, as LazyLock statics. The canonical-purl fast path checks for ?/#/% byte by byte. The output doesn't change: yarn-berry/rescan uses 2.7% fewer instructions. The rest of #1607's gap is binary growth, so the PR says Refs, not Fixes.

Risk: low. It's a local refactor on two pure functions. lines_containing is checked for equivalence against str::lines().filter(..) (CRLF, bare \r, several hits per line, no trailing newline, non-ASCII). The loop body already required @patch: in the value, so it sees the same lines.

Look here:

Verified: I read the full diff and the loop body around the call site. It stays within its scope, has no debug leftovers and leaves CHANGELOG.md untouched. I ran cargo test -p socket-patch-core --lib -- user_patches purl_key locally: 38 passed. CI is green on the head (ci-ok and clippy pass). Bugbot found no issues on d9b8faa.

Changes I made: none.

Open questions: none for this PR. Whether to do a binary-size pass for #1607 is a maintainer call (noted on the issue).

Auto-merge is armed, so approving sends it straight to the merge queue.


Generated by Claude Code

Merged via the queue into main with commit 9ff0e4f Oct 24, 2026
87 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the agent/fix-yarn-berry-rescan-residual branch October 24, 2026 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants