list, str, _collections, _pickle: list strategies, percent formatting, a bounded deque repeat, and a value-keyed pickle memo - #1510
list, str, _collections, _pickle: list strategies, percent formatting, a bounded deque repeat, and a value-keyed pickle memo#1510youknowone wants to merge 14 commits into
Conversation
|
Important Review skippedToo many files! This PR contains 120 files, which is 20 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (120)
You can disable this status message by setting the 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. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 008e576). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d4f4a086d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if matches!(list.strategy, ListStrategy::Empty | ListStrategy::Size) { | ||
| if matches!(other.strategy, ListStrategy::Empty | ListStrategy::Size) { |
There was a problem hiding this comment.
When both lists use Empty/Size storage, this early return incorrectly preserves the receiver's strategy. PyPy's EmptyListStrategy.setslice assigns the donor's strategy and copied storage, so after dst = __pypy__.newlist_hint(5); src = __pypy__.newlist_hint(9); dst[:] = src, the first append should consume the donor's hint of 9, whereas this implementation retains 5; assigning an ordinary empty donor to a Size list similarly should discard its hint. Handle the Empty/Size combinations by adopting the donor rather than returning unchanged.
AGENTS.md reference: AGENTS.md:L223-L226
Useful? React with 👍 / 👎.
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/39134f70ecccad7dd498b7ae9480c605ad7f88ea/pyre-object/src/listobject.rs#L1371-L1372
When a caller supplies a representable but impossible hint, such as items = __pypy__.newlist_hint(sys.maxsize); items.append(1), this new capacity flows into IntArray::with_capacity, whose allocator invokes handle_alloc_error rather than returning a Python error. The Python process therefore aborts instead of raising MemoryError; the float, bytes, ASCII, and object promotion arms have the same problem. Make the hinted allocation fallible and propagate MemoryError through the append path.
AGENTS.md reference: AGENTS.md:L225-L226
https://github.com/youknowone/pyre/blob/39134f70ecccad7dd498b7ae9480c605ad7f88ea/pyre-object/src/listobject.rs#L233-L234
After y = x.copy() on a SizeListStrategy list, both lists deliberately share this state block but have distinct list locks. If one thread calls __pypy__.resizelist_hint(x, n) while another appends to or queries y, this plain i64 store races the plain load in sizehint_state_value, causing undefined behavior in the free-threaded build. Store the shared cell atomically or protect it with synchronization shared by every clone.
AGENTS.md reference: AGENTS.md:L159-L161
https://github.com/youknowone/pyre/blob/39134f70ecccad7dd498b7ae9480c605ad7f88ea/pyre-interpreter/src/builtins.rs#L15989-L15990
For items = __pypy__.newlist_hint(13); items.sort(), PyPy dispatches to the inherited EmptyListStrategy.sort no-op, so the Size strategy and hint remain available for the first append. This fast-path chain handles the other non-object strategies but not Empty/Size, so the generic path clears and reinitializes the list as Empty; the next append consequently allocates the default capacity instead of 13. Add the Empty/Size no-op alongside the strategy sort dispatch.
AGENTS.md reference: AGENTS.md:L225-L226
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… map before reporting its strategy `deque_repeat` and `W_Deque.__imul__` accumulated the whole product in a `Vec` and trimmed it to `maxlen` once at the end, so a bounded deque times a large count allocated the untrimmed length. Both now trim after every round, mirroring the `append`/`trimleft` pair behind `W_Deque.mul`'s `extend`, and take the overflow edge from `ovfcheck`'s machine-signed multiplication rather than from the `usize` accumulator. `mapdict_strategy_repr` returned nothing for an instance whose map had not been installed yet, so `__pypy__.strategy()` raised TypeError for a freshly constructed object; `user_setup` installs the terminator while building the instance. extra_tests/snippets/stdlib_collections_deque_repeat.py covers the bounded repeat, the short-circuiting counts, and the overflow edge. Assisted-by: Claude
`memoize` recorded only the wrapper's identity and `save` resolved the memo by pointer identity against a freshly read memo-list element. An unboxing list strategy stores the erased rpython string and wraps a fresh object per read, so two slots naming one string reach the memo as different objects: `data = [str(i) for i in range(257)]; data.append(data[-1])` pickled its repeated element twice instead of emitting a GET, and unpickling produced two objects where one is expected (`test.test_pickletools` `test_optimize_long_binget`, `PASS -> FAIL` since the AsciiListStrategy port). `interp_pickle.py` keeps `str_memo` and `bytes_memo` beside the identity memo, keyed on `space.utf8_w` / `space.bytes_w`, and `save` consults them before it. Port both, seeded from the entries a reused `Pickler` carries into `dump`. Assisted-by: Claude
`is_build_class_builtin` was the one `BuiltinCodeFn` comparison still spelled `std::ptr::fn_addr_eq`; the nine other comparisons in this file go through `gateway::builtin_code_fn_eq`. Assisted-by: Claude
Both tests install the same capture-free compile callback and read one process-global `COMPILES`, and Rust runs sibling tests concurrently, so one test's reset/run/read interval could observe the other's compiles. A mutex keeps each interval indivisible. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/84ff43bc1ec1b99a1990e6322b67ad55bebc2c2d/pyre-interpreter/src/module/_pickle/pickler.rs#L1329-L1331
When a graph contains two equal but distinct exact strings or bytes, this value-based lookup emits a GET for the second object solely because its contents match the first. For example, round-tripping two separately constructed large equal strings changes result[0] is result[1] from false to true, corrupting the graph's aliasing compared with CPython 3.14; memoization must distinguish separate backing-storage identities while still recognizing wrappers for the same stored object.
AGENTS.md reference: AGENTS.md:L187-L190
https://github.com/youknowone/pyre/blob/84ff43bc1ec1b99a1990e6322b67ad55bebc2c2d/pyre-object/src/listobject.rs#L3820-L3821
When clearing an Ascii-strategy list while another thread advances GC, UnicodeArray::install drops the outgoing block and its try_gc_owns_object call is a safepoint that can relocate the rooted list. The subsequent strategy, length, and allocation stores then use the stale list reference, so the live list can remain uncleared or retain inconsistent storage; re-read root_base after this install as switch_to_object_strategy already does after each storage teardown.
AGENTS.md reference: AGENTS.md:L159-L161
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Four wasm fixtures stopped matching their snapshot, which is what
`pyre/check.py dynasm (ubuntu-24.04)` reported (`wasm 4 failed, 490
passed`; dynasm itself 501/501):
exception_catching_frame_tb_node bridges_compiled 3 -> 2
exception_reentry_guard_finally_residual bridges_compiled 12 -> 11
list_append_virtual_payload bridges_compiled 7 -> 6,
guard_failures 1403 -> 1203
gc_bug_bridge_flavor_traceback_names guard_failures 1054 -> 1038
Only those counters are edited; every other key keeps its recorded value.
The numbers are the ones that run observed on this branch's head.
Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 008e5766a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| let action = crate::executioncontext::space_user_del_action(); | ||
| if !action.is_null() { | ||
| pyre_object::gc_hook::try_gc_collect_oldgen(); |
There was a problem hiding this comment.
Whenever any object anywhere in the process has a registered finalizer, gc_has_pending_finalizers() remains true, so closing or clearing an unrelated generator now performs a whole-old-generation collection and immediately runs every newly discovered __del__. For example, an unreachable finalizable cycle left under gc.disable() will be finalized merely because an unrelated generator is closed; even a reachable long-lived finalizable object causes every generator close to pay for a full heap scan. Neither CPython's frame-local refcount release nor PyPy's frame_is_finished introduces this unrelated collection/callback boundary, so the eager behavior needs to be scoped to objects actually released from this frame rather than using the global finalizer census.
AGENTS.md reference: AGENTS.md:L187-L191
Useful? React with 👍 / 👎.
1 participant
Rebased onto current
main. Thirteen commits.List strategies
list: port PyPy AsciiListStrategy—listobject.py'sAsciiListStrategywith
unicode_array.rsas its erased[rpython str]storage, plus thejit-trace descrs, helpers and specialisation arms that reach it, and the
W_UnicodeObject.is_w/immutable_unique_idstorage-identity arms there-wrapping makes observable.
list: port PyPy SizeListStrategy— the storage-less strategy that carriesan allocation hint until the first append selects a concrete one.
extra_tests/snippets/pypy_list_sizehint.pywalks__pypy__.newlist_hintintoeach of the five concrete strategies and checks the physical size the hint
bought.
list: port PyPy range list strategies—SimpleRangeListStrategyandRangeListStrategy.bench: reconcile list strategy stats after main rebase— jitstats only.Percent formatting
str, bytes: preserve percent format operand order,str, bytes: defer incomplete percent format errors,str, bytes: defer oversized percent quantities,str, bytes: preserve incomplete percent acquisition order— fourformatting.rsfixes to%-formatting.PR #1471 review follow-ups
_collections, mapdict: trim the deque repeat per round, and install a map before reporting its strategy— two findings, both reproduced first:
deque_repeat/W_Deque.__imul__Vecand trimmed tomaxlenonce at the end, sodeque([1, 2], maxlen=2) * (2**40)allocated the untrimmed length instead of answeringmapdict_strategy_reprclass C: pass; __pypy__.strategy(C())raisedTypeErroruntil some attribute was touchedinterp_deque.py'sW_Deque.mulbuilds its answer by extending amaxlen-bounded copy, and everyappendbehind thatextendrunstrimleft— the product is never materialised. Both entry points now trim after each
round the same way, and take the overflow edge from
ovfcheck'smachine-signed multiplication rather than from the
usizeaccumulator, whichdoes not overflow where the signed one does.
imulkeeps the upstream shape ofstarting from
selfand extendingnum - 1times.user_setupinstalls the type's terminator while building the instance, so anobject carrying the mixin never holds a null map; pyre installs it on first
touch, and the diagnostic now asks for it rather than observing the difference.
extra_tests/snippets/stdlib_collections_deque_repeat.py(gated) covers thebounded repeat at counts that exhaust memory under the old accumulator, the
short-circuiting counts, and the
MemoryErroredge. It is written to PyPy'sobservables, not CPython's:
_collectionsmodule.c'sdeque_inplace_repeatreduces the repetition count so that
deque([1], maxlen=1) * (2**62)answersinstantly, while upstream loops — the snippet asserts only what upstream also
answers.
The eleven remaining findings from that review were verified and rejected; the
four GC "reload the owner across a safepoint" ones rest on a mechanism pyre does
not have (
alloc_with_type_no_collectandalloc_in_oldgennever collect, andthe only park point is
gc_sync::safepoint_poll, called from the two eval loopsand the dynasm assembler). Six stale comments still assert the opposite premise
and cite a
gc_op_slowthat does not exist; they are left for a separate pass.The one CI regression this branch had
_pickle: memoize str and bytes by value—memoizerecorded only thewrapper's identity and
saveresolved the memo by pointer identity against afreshly read memo-list element, so once
AsciiListStrategylanded, two listslots naming one erased rpython string reached the memo as different objects:
data = [str(i) for i in range(257)]; data.append(data[-1])pickled itsrepeated element twice instead of emitting a GET, and unpickling produced two
objects where one is expected (
test.test_pickletoolstest_optimize_long_binget,PASS -> FAIL). Measured first: PyPy 3.11.13 andCPython 3.14.2 both answer
unpickled[-1] is unpickled[-2]→True.interp_pickle.pykeepsstr_memoandbytes_memobeside the identity memo,keyed on
space.utf8_w/space.bytes_w, andsaveconsults them first; bothare ported, seeded from the entries a reused
Picklercarries intodump.Note the deliberate consequence, which is upstream's: two equal but distinct
strings now share a memo entry, so unpickling returns one object for both where
CPython returns two. That is
interp_pickle.py's7817dc4eec4 use the byte/str underlying w_bytes/w_str for memoizing, whosecomment names the same cause — "PyPy list strategies wrap stored values in new
W_* objects on each getitem, so identity checks miss even for repeated
references to the same logical value".
Rest
generator: finalize locals at close boundary—collector.rs,baseobjspace.rs,pyframe.rs.builtins: compare the build-class code through builtin_code_fn_eq—is_build_class_builtinwas the oneBuiltinCodeFncomparison still spelledstd::ptr::fn_addr_eq.majit: serialize the two label-entry deopt tests on their shared counter—both install the same capture-free compile callback and read one process-global
COMPILES, and Rust runs sibling tests concurrently.Codex parity review
The three findings filed against the commits above do not hold:
pickler.rs"the value memo can return a GET beforereducer_overrideruns" —
dispatch_savesends exactstr/bytestosave_str/save_bytesbefore the
reducer_overrideblock, so that hook never sees either type,memo or no memo. Measured on CPython 3.14.2: a
Picklersubclass whosereducer_overriderecords what it is offered sees nothing at all for[str, str, bytes, int, float].getindex_repeat(__index__vsint_w) anddeque.__doc__— absentfrom this PR's diff; both come from
b08493ae325(jit-trace, _weakref: gate the class-attribute fold on a fold census, and reject repr with a foreign self #1471), already onmain.Verification:
cargo check -p pyre-interpreter --features dynasmandcargo check -p majit-metainterp --tests --no-default-features --features dynasmboth clean; the new deque snippet passes under CPython 3.14.2. Release binaries
were not rebuilt here — the LLBC extraction was invalidated three times by the
tree moving under it — so the two-backend runs are CI's. The previous head's
run was cancelled at 31 minutes with every long job killed, so the pickle fix
has not had a CI verdict yet.