GeezerVison is showing github.com — Use populated entries for unpickler MEMOIZE indexes by youknowdot · Pull Request #8959 · RustPython/RustPython
Conversation
youknowdot
commented
Oct 4, 2026
•
edited by coderabbitai
Bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
A sparse memo entry changes the index chosen by MEMOIZE: after BINPUT 7, the next implicit memo index must be 1, not 8. The vector length currently makes a subsequent BINGET 1 fail.
Store populated memo entries in a map and choose the implicit index from their count. Preserve ascending memo.copy() order, reserve storage fallibly, and release displaced values after memo guards end. Add one bounded, unique sparse/MEMOIZE regression after comparing coverage with the canonical pickle tests.
This is extracted from #8954 onto current main's unchanged Python 3.14 target. It changes no pickle protocol version or 3.15 language behavior. The mega PR retains the implementation until this independent change is merged.
Fresh standalone validation on cccc67bdf9929948976933c3b9a678a346e8bf06:
- Rust workspace: 1,345 passed /18 ignored; separate C-API: 116 passed /4 ignored
- Both Clippy gates pass without warnings; release startup confirms Python 3.14 and marshal/pickle version 5
test_pickle: 1,008 run, 73 skipped, 5 retained expected failures; test_pickletools: 190 run, 14 skipped, 1 retained expected failure. Both complete successfully- New sparse regression passes native RustPython and exact CPython 3.14.7
- Five bounded native contract groups and four memo-finalizer reentry operations pass, including copying the memo during clear, replacement, overwrite and reinitialization
- Full snippets: 470 passed /12 environment failures. Native 235/4, CPython 3.14.7 reference 235/4, host harness 0/4. The failing multiprocessing/ownership cases have the same AF_UNIX/UID-map restrictions on both interpreters; no source skips or assertions were weakened
- Normal configured commit hooks pass
CPython 3.14.7 agrees on sparse opcode/MEMOIZE and protocol roundtrips. Its explicit sparse dictionary-assignment/copy cases omit entries; those two cases are checked as preservation of existing RustPython behavior, rather than claimed reference parity. This patch does not establish general unpickler GC or recursive-load safety.
AI assistance: prepared, reviewed and tested with Codex at the user's direction. Exact model metadata is unavailable; the commit records Assisted-by: Codex:model-version-unavailable.
Summary by CodeRabbit
- Bug Fixes
- Fixed pickle loading for streams with gaps in memo indices, preserving shared object references and correctly assigning subsequent memo entries.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
A sparse BINPUT followed by MEMOIZE must use the number of occupied memo
entries, not the largest index plus one. Store populated entries in a map,
preserve ascending memo copies, reserve fallibly, and release displaced
references after memo guards end.
Keep existing memo assignment/proxy behavior and add one bounded regression
for sparse BINPUT, MEMOIZE identity and copy order. No protocol version,
GC traversal or unrelated pickle input changes are included.
Extracted from the Python 3.15 migration onto the unchanged 3.14 target.
Assisted-by: Codex:model-version-unavailable
coderabbitai
Bot
commented
Oct 4, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius.
No actionable comments were generated in the recent review. 🎉
ℹ️ Recent review info
⚙️ Run configuration
- Configuration used: Repository: RustPython/RustPython/.coderabbit.yml
- Review profile: CHILL
- Plan: Advanced
- Run ID:
240687a7-2dac-4807-b02d-a6c85ddf1ff7
📥 Commits
Reviewing files that changed from the base of the PR and between cccc67b and f4c7ea1.
📒 Files selected for processing (1)
extra_tests/snippets/stdlib_pickle.py
💤 Files with no reviewable changes (1)
- extra_tests/snippets/stdlib_pickle.py
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
📝 Walkthrough
Walkthrough
Unpickler memo storage now uses a sparse map. Memo opcodes and proxy operations use the updated representation, and a regression test checks memo holes, keys, and object identity.
Changes
Pickle memo handling
| Layer / File(s) |
Summary |
Sparse memo representation and helpers
crates/stdlib/src/pickle.rs |
The unpickler memo uses a HashMap with fallible copy and insertion helpers. Constructors initialize an empty map. |
Memo proxy and replacement lifecycle
crates/stdlib/src/pickle.rs |
Proxy copies return entries in index order. Dictionary assignment validates nonnegative integer keys and reports allocation failures as MemoryError. Clearing, reinitializing, and replacing the memo take old contents out of the lock before dropping them. |
Memo opcode writes
crates/stdlib/src/pickle.rs, extra_tests/snippets/stdlib_pickle.py |
Memo write opcodes use the updated insertion path. MEMOIZE selects an index from the number of memo entries. The regression test checks sparse keys and shared object identity. |
Priority: ⬇️ Low
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Suggested reviewers: youknowone
Merge Risk: ⚪ Minimal · up to f4c7e
No actionable issue is established for the selected change; it is mergeable after normal checks.
Security Architecture Review
Security architecture risk: 🔵 Low · up to cccc6
The change keeps memo ownership within each unpickler, avoids allocation for sparse-index gaps, and releases displaced objects outside memo locks. No introduced security vulnerability was identified. Residual uncertainty concerns broader finalizer behavior and recovery after failed loads, rather than a new privilege or service boundary.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
- inferred — The directly changed state is one unpickler's memo and its referenced Python objects within the interpreter process. Sparse storage does not establish an isolation boundary: broader object-resolution authority remains dependent on the existing unpickling hooks and caller context.
Security Findings and Attack Paths
- inferred — The inspected memo changes do not introduce a new authority-bearing sink. Pickle-controlled indices select existing memo references or store the stack value, while absent entries fail. Existing class-resolution and persistent-load hooks remain outside the changed memo implementation.
Trust Boundaries and Controls
- observed — The public memo setter rejects deletion, unsupported value types, noninteger keys, and negative representable indices. It builds a separate replacement map before swapping the owner's live memo. Memoize count selection and insertion share one write guard, avoiding a count-to-insert race.
Resilience and Maintainability Implications
- observed — A large explicit memo index now reserves only an entry rather than vector gaps. Copy and replacement reserve according to populated-entry counts and map reservation failures to MemoryError. This reduces sparse-index allocation amplification, but does not bound total entries supplied by a pickle or dictionary.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name |
Status |
Explanation |
Resolution |
| Docstring Coverage |
⚠️ Warning |
Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 3 files. |
Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name |
Status |
Explanation |
| Description Check |
✅ Passed |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check |
✅ Passed |
The title clearly states the main change: MEMOIZE indexes use the count of populated unpickler memo entries. |
| Linked Issues check |
✅ Passed |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check |
✅ Passed |
Check skipped because no linked issues were found for this pull request. |
- Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
❤️ Share
Comment @coderabbitai help to get the list of available commands.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Merging this PR will not alter performance
✅ 62 untouched benchmarks
⏩ 4 skipped benchmarks1
Comparing youknowdot:python314-sparse-memo (cccc67b) with main (f39b054)
|
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.
stdlib_pickle.py
do not add new test file without good reason
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.
Renamed it to extra_tests/snippets/stdlib_pickle.py in f4c7ea16. There was no existing stdlib_pickle.py on this branch, so this is a 100% rename: the regression's contents and assertions are unchanged. Test discovery and execution under the new name pass with both RustPython and CPython 3.14.7.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Use the module-level snippet name requested in review without changing the regression assertions.
Assisted-by: Codex:model-version-unavailable
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.
|