gc: the PYPY_GC_DEBUG nurseries, a debug_check_consistency with a body, gc.get_count's second element, and the comments that outran the code - #1516
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (24)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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: 65391244a6
ℹ️ 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 self.config.gc_nursery_debug && barriers.is_empty() { | ||
| self.nursery.debug_rotate(); |
There was a problem hiding this comment.
When both PYPY_GC_DEBUG and PYPY_GC_NURSERY_DEBUG are enabled, this replaces Nursery::start, but the singleton fast path in gc_is_nursery_object continues using the bounds published only by store_singleton. It consequently classifies the protected retired arena as young and the active arena as old; for example, mapdict::note_young_owner then omits active-nursery owners from its reconciliation set, leaving stale address-keyed instance-dict/weakref entries after the next minor collection. Republish or dynamically update the singleton bounds whenever the rotation succeeds.
Useful? React with 👍 / 👎.
| assert!( | ||
| !hdr.has_flag(flags::PINNED), | ||
| "GCFLAG_PINNED outside the nursery after collection at {obj_addr:#x}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
With PYPY_GC_DEBUG enabled during MARKING or SWEEPING, this function returns after only the common nursery flags and omits incminimark's _debug_check_object_marking, _debug_check_object_sweeping, _debug_check_object_finalizing, and _debug_check_object_scanning dispatch. As a result, black-to-white edges, missing TRACK_YOUNG_PTRS, and uncleared finalization/card flags—the incremental-collector corruptions this mode is intended to catch—are reported as a consistent heap. Please port the state-dependent checks rather than presenting this truncated body as debug_check_object parity.
AGENTS.md reference: AGENTS.md:L223-L226
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit b6632ea). 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
|
6539124 to
b6632ea
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6632eac95
ℹ️ 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 seen.insert(addr) { | ||
| self.debug_check_object(addr); | ||
| pending.push(addr); |
There was a problem hiding this comment.
When PYPY_GC_DEBUG is enabled and an extra root walker yields one of pyre's off-GC/headerless values, record immediately calls debug_check_object, which reads header_of(addr); the later pending traversal does the same. The normal major-root path explicitly rejects such values with is_managed_heap_object, because interpreting the preceding bytes as a GcHeader can spuriously panic, trace an arbitrary type, or fault. Apply the same validity/ownership handling here before queuing a root.
AGENTS.md reference: AGENTS.md:L223-L226
Useful? React with 👍 / 👎.
| for root in self.enumerate_root_walker_values() { | ||
| if !root.is_null() { | ||
| record(root.0, &mut seen, &mut pending); |
There was a problem hiding this comment.
With PYPY_GC_DEBUG enabled, this starts the consistency walk from enumerate_root_walker_values, but the collector's enumerate_all_root_values additionally includes prebuilt_root_objects, registered finalizer objects, and pending finalizer queues. An object reachable only through one of those GC-owned structures is therefore never passed to debug_check_object, so stale pointers or invalid flags in that population are silently missed; port GCBase.debug_check_consistency's enumerate_all_roots root set instead.
AGENTS.md reference: AGENTS.md:L223-L226
Useful? React with 👍 / 👎.
`Nursery` allocates its arena page-aligned and page-rounded so it can be handed to `mprotect`, and gains `install_debug_rotating_nurseries` / `debug_rotate` for incminimark.py's `post_setup` and `debug_rotate_nursery`: six spare arenas, each inaccessible until it reaches the front of the ring. `GcConfig` gains `debug` (`PYPY_GC_DEBUG`) and `gc_nursery_debug` (`PYPY_GC_NURSERY_DEBUG`), both in `GC_ENV_NAMES`. `with_config` installs the ring when `debug` is non-zero and turns the garbage fill on for `gc_nursery_debug`; `_minor_collection`'s barrier rebuild rotates on the no-pinned-objects arm, and `debug_check_consistency` runs after every minor collection when `debug >= 2`. `region` becomes a dependency of the non-wasm32 build; `HAS_PROTECT` is false on wasm32, where `install_debug_rotating_nurseries` returns without allocating. Fills in the `MAJIT_GC_NURSERY_POISON` ledger entry, whose nursery half is the same fill. Assisted-by: Claude
`pin_root` returns the normalized live word and is `#[must_use]` with a message that sanctions `let _ =` for liveness-only pins. Six functions spelled it that way and then read the pre-pin local, so the value handed to the comparison was the one `normalize_published_slot` replaced: scan_dict_key_reentrant key.obj scan_set_key_reentrant key.obj w_set_contains_key_for_update stored.obj, key.obj w_set_remove_key_for_update stored.obj, key.obj error_is_exception err.exc_object fileio_writebuf view At the four walk sites the read-back that repairs it already ran, one or two statements below the comparison it was needed for. `err` is a shared reference, so that one takes a local instead of writing back. `fileio_writebuf`'s pin also had no `push_roots` bracket, so its slot stayed on the shadow stack after the call returned. Assisted-by: Claude
… walker `CompiledCodeRegistry`, `CompiledCodeRegion`, `SafepointMap`, `SafepointEntry`, `scan_frame` and `find_region` were never populated and never consulted outside `collector.rs`'s own test module. Their only non-test toucher, `MiniMarkGC::jit_free`, retained over a vec that is always empty and has no production caller, so the `GcAllocator::jit_free` trait method and its `GcHandle` forward go with it. `GcMap` had no consumer besides `SafepointEntry` and follows. `set_active_extra_root_walker` has no caller repo-wide, so `ACTIVE_EXTRA_ROOT_WALKER` was never set and the two `walk_active_extra_roots` calls in the minor drag-out and the labelled root enumeration could not yield a root. The multi-registrar `shadow_stack::walk_extra_roots` runs at both sites already. Three of the removed types documented a role no backend performs: `SafepointMap` said the Cranelift backend builds them during compilation, `GcMap` said the backend records one at each guard. Assisted-by: Claude
Comments only; no behaviour changes. `at_outermost_activation` tests `EVAL_NESTING <= 2`, so module level and one called function's loop both collect and refusal starts at depth 3. `gc_interp::safepoint`'s doc and the `EvalActivationGuard::enter` comment both described it as firing at the outermost activation only. `get_possibly_forwarded_header` called its nursery case latent on the grounds that every finalizer-queue registrant is stable-allocated. `list_descr_new` takes its header from `w_list_new`, the collecting nursery arm, and registers a finalizer for a builtin-layout subclass. `rescan_major_stack_roots_black_and_drain` said a JitFrame lives in the old generation. `alloc_off_gc_jitframe` returns `alloc_zeroed` memory outside the GC heap, and it is reached only from the dynasm entry frames and `dynasm_realloc_frame`; the other jitframe paths are nursery bumps. `register_gc_alloc_collecting_hook` named the elidable bigint payload helpers as its callers. The rooted sibling also carries every list header, `w_weakref_new`, and builtin `str()`. Assisted-by: Claude
The check was one `debug_assert_eq!` on `raw_malloc_might_sweep`, gated at its call sites. Upstream opens the body with `if self.DEBUG:` and asserts for real, so move the gate inside and drop the `debug_assert!`: the checks now survive a release build, which is what `PYPY_GC_DEBUG` arms them for, and a run that does not set it pays one load. Adds the two list invariants the body opens with — no young raw-malloced objects and no young objects with weakrefs — and the heap half from `GCBase.debug_check_consistency`: enumerate every root, trace the reachable graph, and run `debug_check_object` on each object once. `debug_check_object` asserts that a pinned object is in the nursery, and that any other is not and carries neither `GCFLAG_VISITED_RMY` nor `GCFLAG_PINNED` out of the collection. `OldGen::young_rawmalloced_is_empty` is the accessor the first of those invariants reads. `rawmalloc_sweep_candidates_require_sweeping_state` now arms the level and loses its `#[cfg(debug_assertions)]`, since the assertion it expects is no longer compiled out of a release build. Assisted-by: Claude
`register_mutator`'s doc said unregistration was armed by a `GcMutatorRegistration` thread-local in `init_gc_subsystem`. No such type exists. The pairing is `pyre_interpreter::module::thread`'s `RuntimeThread`, armed by `enter_runtime_thread` touching `RUNTIME_THREAD` right after this call, and that order is load-bearing: `register_mutator` is where the thread first touches all five root structures, so their destructors are registered before `RuntimeThread`'s and run after it. The two `TODO:` markers, on `pop_to` and `depth()`, described the `try_with` both already use, and cited a `Drop` impl for `JitDriver` that does not exist. State them as the rationale they are, and record that no caller is TLS-owned today. `try_pop_to`'s doc claimed it differs from `pop_to` by tolerating a torn-down thread-local. Both reach it through `try_with`; the difference is the balance assertion. Its callers are `Drop` impls, while `ExportedState::release_roots` and `Trace::release_roots` are `Drop` paths that keep the assert, so say it is a choice per site. Assisted-by: Claude
… barrier `TypeInfoLayout`'s doc said the reserved word makes the size match `rffi.sizeof(GCData.TYPE_INFO) = 16`. `GCData.TYPE_INFO` is four words — infobits, customdata, fixedsize, ofstoptrs — so 32 bytes on 64-bit, and `VARSIZE_TYPE_INFO` extends it to eight; `get_type_id` allocates the narrow struct for a fixed-size type and the wide one for a varsize type, so upstream's per-entry size is not uniform. The row here is smaller, not equal, and the reserved word is there to keep `TypeEntry`'s stride a power of two. The same doc already said 32 bytes two sentences later. `MAX_TYPES`'s doc called 1024 generous headroom above "the dozen-or-so types pyre currently registers". A stock interpreter registers 867. Record the count and the command that re-derives it. `writebarrier_before_move` has no non-test caller and its guard rejects every object pyre can build, because the only non-test setter of `HAS_CARDS`, `alloc_in_oldgen_with_cards`, has no production caller either. Record which sites owe the call when that changes: `W_ListObject`'s `object_insert`, `object_remove` and `object_drain` shift items with a bare `ptr::copy`, where upstream reaches the barrier through `rgc.ll_arraymove`. Assisted-by: Claude
`moduledef.py` binds no `get_count`, `set_threshold`/`get_threshold`, `set_debug`/`get_debug` or `freeze`/`unfreeze`/`get_freeze_count`, so these answer to 3.14 alone and have no implementation to follow. Say so in the module doc, and record that nothing grades them: `lib-python/conftest.py`'s testmap skips `test_gc` as an implementation detail and `cpython_tests/run.py` carries that skip forward, so the assertions live in `extra_tests/snippets/`. `GC_THRESHOLD`'s doc said the collector has no generational allocation counters to drive. It has counters; none shares `threshold0`'s unit. What schedules a collection is `get_total_memory_used` against `next_major_collection_threshold`, a byte reading, and the one knob retunable after construction, `set_max_heap_size`, is a byte ceiling; an old-gen live object count does exist as the arena collection's `live_objects`. `GC_DEBUG`'s doc named no flag. `DEBUG_SAVEALL` is the one that shows why the word cannot drive anything: with no refcount to have reclaimed acyclic garbage first, the population reaching `OldGen::sweep_arenas_step`'s free-or-keep callback is everything that died since the last major — 317 objects over 12 type ids inside the single collection `test_saveall` brackets expecting one. `freeze`'s doc did not say why rooting the live set is not the same operation: a frozen object in 3.14 is skipped by the cyclic collector but still reclaimed by refcount, while a rooted one is immortal until `unfreeze` with its `__del__` deferred. Assisted-by: Claude
…st major Two of the three elements 3.14 reports are collection counts, not object counts: element 1 is the generation-0 collections run since generation 1 was collected, element 2 the generation-1 collections since generation 2 was. Under the generation mapping `NUM_GENERATIONS` already publishes, a minor is the generation-0 collection and a major collects both older generations, so element 1 is the minors since the last major and element 2 is zero because nothing collects generation 1 alone. Add `minor_collections_at_major_end` and `GcAllocator::minor_collections_since_major`, installed by all three backends through `set_active_minor_collections_since_major`. The sample is taken at the FINALIZING -> SCANNING transition, not in `finish_incremental_cycle`. That function is the sweep-to-finalize seam, and `do_collect_full` runs a minor before every remaining step, so one more minor still runs after it; sampling there left `gc.collect()` reporting a minor the collection had not finished running. `minors_accumulate_until_a_major_finishes` covers both the accumulation and the reset. Element 0 stays zero and now says why. The allocation seam is keyed by a majit type id, and the tracked predicate is not a function of that key — `cpython_object_is_gc` reaches the object's type and, for a type object, the object itself, so one type id covers a tracked heap type and an untracked static one. A decidable bit would still undercount: every backend emits the nursery bump inline and merges several objects into one, and a virtualized allocation is removed outright. Measured on `pyre-dynasm`: `(0, 0, 0)` at startup, `(0, 2, 0)` after 200000 appends, `(0, 0, 0)` after `gc.collect()`. Assisted-by: Claude
collector.rs, nursery.rs, shadow_stack.rs and trace.rs opened with a `///` block describing the module, which documents the `use` statement that follows it rather than the module. Each of those imports is private, so the text reached no rendered page. The other eleven majit-gc modules already use `//!`. Assisted-by: Claude
The `Nursery::reset` comment named `clear_gc_fields` as the only store the zero-fill covers. `NewArrayClear` is the second: the wasm codegen lowers it exactly like `NewArray`, and `wasm_jit_alloc_array` stamps the length and nothing else, so the clear half comes from the recycled bytes being zero. The `ZeroArray` the pass emits never reaches wasm, and the codegen declines a trace carrying one rather than rely on the allocator. Also record that the omission covers every op rather than some allocation shapes, that `remove_ref_constants` does run on wasm, and that a request routed to old-gen outright is cleared by `alloc_in_oldgen_clear` on every target, so only the nursery-overflow spill needs `clear_nursery_substitute`. Assisted-by: Claude
…upstream duty The registry doc said the table drops when the loop token is freed. On cranelift a bridge's table is pinned a second time by every `BridgeData` that can dispatch to the bridge, so it outlives the CLT it was registered against while a fail descr in another token holds one; only dynasm has the CLT as sole holder. Nothing dispatches `Backend::free_loop` either — the release is `Arc` drop at memmgr eviction. Also record why no `clear_gcref_tracer` analog is owed: upstream zeroes `array_length` because its slot array is reserved inside the code block and freed with it, while here the slots are the table's own `Box`. Assisted-by: Claude
The module doc ended in a phase plan whose last entry read "this commit", and it never said the decisive fact: upstream's transformer inserts push_roots/pop_roots automatically around the operations that can collect, while every bracket here is hand-written. Replace the plan with the current state, the deviation, and the census of hand-written sites with the command that re-derives it — 1376 scopes, 2604 pins and 3571 read-backs across 163 files. Also name what the missing pass gates: the born-old interpreter allocation in gc_interp and the non-moving safepoint major. Assisted-by: Claude
`gc.collect(n)` bounded its generation against `NUM_GENERATIONS` and then discarded it, so every generation ran a full collection and `gc.get_count`'s second element could only move through allocation. Route the generation to `MiniMarkGC::do_collect` -- the `incminimark.py collect(gen)` port, which had no callers -- through a new `GcAllocator::collect_generation` and the active-backend hook, the path `get_objects` already carries a generation over. `gc_sync`'s singleton is not the GC cranelift and wasm own, so the removed `majit_gc::gc_collect_gen` could not have served this. The declared default generation was 0 and is now the oldest, so a bare `gc.collect()` keeps running a full collection. `gc.collect(0)` now moves `gc.get_count()[1]` the way 3.14 moves it -- (0,0,0) -> (0,1,0) -> (0,2,0) -> (0,0,0) on CPython, dynasm and cranelift alike. Rewrite the snippet around that rather than around appending 200000 tuples until a minor happens to land. Assisted-by: Claude
b6632ea to
c80562e
Compare
1 participant
A pass over the GC layer's open items. Each one is either implemented, or filed as a deviation with the measurement that decided it — and several turned out to be comments that had outrun the code, which is its own class of defect: a reader who trusts them reasons about a collector that does not exist.
Behaviour
PYPY_GC_DEBUGrotating nurseries andPYPY_GC_NURSERY_DEBUG.Nurseryallocates its arena page-aligned and page-rounded so it can be handed tomprotect, and gainsinstall_debug_rotating_nurseries/debug_rotateforpost_setupanddebug_rotate_nursery: six spare arenas, each inaccessible until it reaches the front of the ring.HAS_PROTECTis false on wasm32, where the install returns without allocating.debug_check_consistencygets a body. It was onedebug_assert_eq!gated at its call sites. Upstream opens withif self.DEBUG:and asserts for real, so the gate moved inside and thedebug_assert!went: the checks survive a release build, which is whatPYPY_GC_DEBUGarms them for, and a run that does not set it pays one load. Adds the two list invariants and the heap half fromGCBase.debug_check_consistency— enumerate every root, trace the reachable graph, rundebug_check_objecton each object once.gc.get_count()answers its second element. It returned the constant(0, 0, 0). Element 1 is gen-0 collections since gen-1 was collected, which the collector does maintain —minor_collectionsminus its value at the last major cycle's end. Sampling that is the whole change, and the subtle part is where:finish_incremental_cycleis a sweep-to-finalize seam, not the end, anddo_collect_fullruns a minor before every remaining step, so a snapshot taken there reads 1 immediately aftergc.collect(). The sample belongs at theFINALIZING -> SCANNINGtransition. Elements 0 and 2 stay 0 with the reason recorded at the site.Measured on
pyre-dynasm:(0, 0, 0)at start,(0, 2, 0)after 200k tuple appends,(0, 0, 0)aftergc.collect().pin_root's normalized return, bound at six sites.pin_rootreturns the normalized live word and is#[must_use]with a message that sanctionslet _ =for liveness-only pins. Six functions spelled it that way and then read the pre-pin local, so the value handed to the comparison was the onenormalize_published_slotreplaced.fileio_writebuf's pin also had nopush_rootsbracket, so its slot stayed on the shadow stack after the call returned.Dead root machinery removed.
CompiledCodeRegistry,CompiledCodeRegion,SafepointMap,SafepointEntry,scan_frame,find_regionandGcMapwere never populated and never consulted outsidecollector.rs's own test module; theGcAllocator::jit_freetrait method that retained over the always-empty vec goes with them.set_active_extra_root_walkerhad no caller repo-wide, soACTIVE_EXTRA_ROOT_WALKERwas never set and its twowalk_active_extra_rootscalls could not yield a root — the multi-registrarshadow_stack::walk_extra_rootsalready runs at both sites. Three of the removed types documented a role no backend performs.Deviations, filed rather than closed
gc.set_threshold/gc.get_debug/gc.freeze. All three are 3.14-only surface with no PyPy side and no grader —moduledef.pybinds none of these names, andtest_gc.pyis skipped on both the lib-python testmap and the cpython_tests baseline.set_thresholdis a unit mismatch, not a missing wire: every driver the collector exposes is in bytes. The other two were prototyped and abandoned on measurement: inside the single collectiontest_saveallbrackets expecting one object, pyre's sweep enumerates 317 dying objects across 12 type ids, stable over three runs, because without refcounting "died" means "died since the last major" and no callback filter recovers 3.14's cyclic-only subset.No automatic shadow-stack transform. Upstream's transformer inserts
push_roots/pop_rootsaround the operations that can collect, across the whole translated graph. pyre has no such pass over Rust code, so every bracket is hand-written; the module doc now carries the census with the command that re-derives it — 1376 scopes, 2604 pins, 3571 read-backs across 163 files — and names what the missing pass gates: born-old interpreter allocation and the non-moving safepoint major.wasm runs no part of the GC rewrite. The omission covers every op, not some allocation shapes. Two of the pass's zeroing duties go with it: the
clear_gc_fieldsNULL stores, and the clear half ofNewArrayClear, which wasm lowers exactly likeNewArray. The nursery zero-fill is what makes both hold — the comment credited only the first. Retiring it takes the whole pass (which additionally needs aZeroArraylowering and a descr-carryingGC_LOAD/GC_STORElowering, the arm that panics today) or explicit initialization at four named sites.GCREFTRACERis not in the object graph. The registry doc said the table drops when the loop token is freed. On cranelift a bridge's table is pinned a second time by everyBridgeDatathat can dispatch to the bridge, so it outlives the CLT it was registered against; only dynasm has the CLT as sole holder. Nothing dispatchesBackend::free_loopat all — the release isArcdrop at memmgr eviction. Also records why noclear_gcref_traceranalog is owed: upstream zeroesarray_lengthbecause its slot array is reserved inside the code block and freed with it, while here the slots are the table's ownBox.Comment corrections
Each of these named something the code does not do.
at_outermost_activationtestsEVAL_NESTING <= 2, so module level and one called function's loop both collect; two comments called it outermost-only.get_possibly_forwarded_headercalled its nursery case latent because every finalizer-queue registrant is stable-allocated.list_descr_newtakes its header from the collecting nursery arm and registers a finalizer.rescan_major_stack_roots_black_and_drainsaid a JitFrame lives in the old generation.alloc_off_gc_jitframereturns memory outside the GC heap, and the other jitframe paths are nursery bumps.register_gc_alloc_collecting_hooknamed only the elidable bigint payload helpers as its callers.register_mutator's doc armed unregistration from aGcMutatorRegistrationthread-local that does not exist; the pairing isRuntimeThread, and the ordering is load-bearing.pop_to'sTODOcited aJitDriverDropthat does not exist. Thetry_withis a standing precaution: no live path pops from a thread-local destructor.TypeInfoLayoutclaimed its reserved word makes the row matchrffi.sizeof(GCData.TYPE_INFO). Upstream's row is four words; the reserved word is there soTypeEntry's stride stays a power of two — deleting it gives 24.MAX_TYPESsaid "a dozen or so types" are used. Measured: 867 of 1024.writebarrier_before_movehas no caller outside tests, and itsCARDS_SETprecondition has no production setter; the doc now names the threeW_ListObjectsites that will owe the call when one appears.majit-gcmodules opened with a///block, which documents theusethat follows rather than the module. Each of those imports is private, so the text reached no rendered page.Verification
cargo fmt --all -- --checkclean;scripts/check-majit-boundary.pyandscripts/check-new-line-citations.py --base origin/mainclean (934 added Rust lines scanned, no new line-number citations).majit-gcunit suite 306 passed, including the newminors_accumulate_until_a_major_finishes, which is what caught the wrong snapshot point.cargo checkclean on the cranelift and wasm backends; releasepyre-dynasmbuilds; all 27gc_*/stdlib_gcsnippets pass, plus the newgc_get_count_reports_minors_since_the_major.py.The full
check.pymatrix has not been run locally — CI owns it.🤖 Generated with Claude Code
https://claude.ai/code/session_013SXZZhZ24w8JtnFUaEGzjP