GeezerVison is showing github.com — Read Python locks without copying them in VEX (#1473) by mikolalysenko · Pull Request #1486 · SocketDev/socket-patch
Conversation
LLM Description written by Claude Code:claude-opus-5-5
Refs #1473
This is a partial fix. It makes each poetry.lock read in a hosted scan
cheaper, but the lock is still parsed more than once. #1473 stays open for
the parse-sharing slice (see Follow-ups).
Root cause
A hosted scan on a Poetry project read poetry.lock into an editable
toml_edit::DocumentMut three times, only to look at it:
lock_inventory::pypi::inventory_poetry_lock reads it to list packages.
vex::discover::pypi_locks::extract_poetry reads it in each of the two
VEX discovery passes: pin discovery before the rewrite, and the in-run VEX
over the rewritten files.
DocumentMut::from_str is Document::parse plus despan, which copies
every key, value and comment out of the source into owned strings. Freeing
those copies later costs more again. Callgrind on poetry/hosted (main
4493714, 403M Ir) showed despan at 26.5% and freeing Items at
29.8% inclusive. That is more than the parse itself.
Fix
Discovery and the PyPI lock inventory only read. They now use toml_edit's
read-only Document<&str>, which keeps byte spans into the source. Values
decode the same way, and both document types deref to Table, so the
shared helpers (poetry_lock::lock_version, metadata_files,
python_lock::lock_package_collection) now take &Table. Existing
&DocumentMut callers still compile through deref coercion.
uv.lock, PEP 723 script locks, pylock and pdm.lock go through the same
readers, so they get the same saving.
Evidence
- Callgrind,
poetry/hosted (socket-patch-bench serve fixture, 400
packages): 403.3M → 364.7M Ir (−9.6%). The scan's --json output is
byte-identical between the two binaries.
- New test
vex::discover::tests::read_only_toml_reads_values_like_a_mutable_parse
checks that the read-only parse gives the same values as a DocumentMut.
It covers escaped, literal, multi-line and CRLF strings, dotted and quoted
keys, inline-table arrays and [metadata.files] lookup. This is a
behaviour-preservation test, not a red→green one, because the change
is a performance refactor. The type change (toml_or_diag returns
Document<S>) is what stops the copy coming back.
- Local runs:
cargo fmt --all -- --check: ok.
cargo clippy --workspace --all-features -- -D warnings: ok.
cargo test -p socket-patch-core --all-features --lib: 6444 passed,
4 failed. All 4 are chmod-based tests (copy_tree relax-loop,
vlt_heal unremovable lock, pypi_poetry /
pypi_requirements write-failure rollback). They can't fail a write
when run as root (uid 0) in this sandbox, they don't touch this diff,
and CI runs as non-root.
cargo test -p socket-patch-cli --all-features --lib plus the
e2e_vex_lockfile, in_process_redirect_poetry,
in_process_redirect_pdm, in_process_scan,
covgap_commands_vex, e2e_vex_redirect, mode_migration_pypi and
hosted_superseding_pypi suites: all 1,458 tests pass. The full
--workspace run ran out of sandbox disk building every integration
binary, so CI covers the rest.
/code-review high
It found no correctness bugs. Fixed in 1f6fcc9:
- The
toml_or_diag docs now say which readers still convert back to an
editable document.
- The one-line
read_only_toml wrapper is gone.
is_multiline_array now warns that it needs a DocumentMut, because a
read-only document keeps layout as spans.
Not fixed here, on purpose:
- Cargo.toml / Cargo.lock, pyproject.toml / hatch.toml still convert
with into_mut. Their helpers take &DocumentMut, and Cargo is a
separate p2 bench entry (Slow scan: cargo hosted 3.8x median ms/pkg (per-patch Cargo.lock re-parse) #761). Behaviour and cost are unchanged from
main.
- Other whole-lock
DocumentMut parses used only to read
lock-version (pre_1_4_writer, the pypi_takeover preflight,
lock_writer_version) and the Cargo lock inventory. These are follow-up
candidates now that lock_version takes &Table.
lock_version signature change (&DocumentMut → &Table). It is
source-compatible for every call through deref coercion, and nothing names
it as a function pointer.
Follow-ups (why this is Refs, not Fixes)
After this PR, toml_edit::Document::parse is about 46% of
poetry/hosted Ir across five parses of two texts:
- The original lock is parsed three times: inventory, discovery pass 1, and
the rewrite batch.
- The rendered lock is parsed twice: the rewrite batch's check, and
discovery pass 2.
Sharing one read-only parse per text across those consumers (5 → 2) would
save roughly another 25%. That needs a parse cache or a handoff across the
CLI → engine → discovery boundary, which is its own design.
🤖 Generated with Claude Code
https://claude.ai/code/session_01R8rt7x9KFET52NYm2f3Mbx
Generated by Claude Code
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Assisted-by: Claude Code:claude-opus-5-5
A hosted scan of a Poetry project read poetry.lock into an editable
TOML document three times just to look at it: once to list its
packages and once in each of the two VEX discovery passes. An editable
document copies every key, value and comment out of the file, and on a
large lock that copy and freeing it cost more than the parse itself.
Discovery and the PyPI lock inventory only read, so they now use
toml_edit's read-only document, which points back into the file text
instead. Values read the same; nothing here re-renders an item. uv,
pylock and PDM locks go through the same reader and get the same
saving.
On the poetry/hosted bench fixture this takes a scan from 403M to
365M instructions (-9.6%) with byte-identical JSON output.
Refs #1473
Assisted-by: Claude Code:claude-opus-5-5
Assisted-by: Claude Code:claude-opus-5-5
Use toml_edit's read-only parse directly in the lock inventory instead
of a one-line wrapper, say in toml_or_diag's docs which readers still
convert to an editable document, and warn that is_multiline_array
needs an editable document, since a read-only one keeps layout as
spans and would read every entry as single-line.
Refs #1473
Assisted-by: Claude Code:claude-opus-5-5
cursor[bot] reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 1f6fcc9. Configure here.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|