jit: revalidate the quasi-immutable fields, share one QuasiImmut, drop the loop-invalidation timer (method-cache pair 38 → 39 ops; the 38 was unsound) - #956
Merged
Conversation
`is_quasi_immut_known` / `quasi_immut_now_known` now read and write
`CacheEntry.quasiimmut_seen{,_refs}` (heapcache.py:604-627) and take
`(fielddescr, box)` in upstream's argument order, replacing the flat
`HeapCache.quasi_immut_known` set.
The two `CacheEntry` fields were already ported, along with their
`_clear_cache_on_write` (heapcache.py:70-77) and `_invalidate_unescaped`
(heapcache.py:121-129) clears, but had no readers or writers. The flat
set that stood in for them was cleared only by `reset()`, i.e. at trace
teardown.
`clear_caches_varargs` (heapcache.py:341-370) arms
`need_guard_not_invalidated` and runs `invalidate_unescaped` per entry
for every general call, so with the mark held there a residual call
between two reads of the same quasi-immutable field re-emits
QUASIIMMUT_FIELD, which in turn emits a second GUARD_NOT_INVALIDATED
(heap.py:810-822). On the two-method-call fixture the pre-opt block goes
from one QuasiimmutField to two and the steady body from 38 to 39 ops.
Assisted-by: Claude
The tracer captures the field's value where it records QUASIIMMUT_FIELD
(`QuasiImmutDescr.get_current_constant_fieldvalue`,
quasiimmut.py:135-143) and carries it on the op. OptHeap re-reads the
live value through `get_runtime_field` and returns
`InvalidLoop('quasi immutable field changed during tracing')` when the
two disagree (heap.py:798-804 `QuasiImmutDescr.is_still_valid_for`),
instead of compiling a loop whose baked constant nothing re-proves.
Upstream hangs the captured value on a `QuasiImmutDescr` minted per
recorded op; pyre's descrs are registry-indexed singletons, so it rides
on the op as `arg(1)`, the way the `record_namespace_quasiimmut_field`
twin already carries its slot index.
`w_type_notify_quasi_immut_watchers` unlinks `quasi_immut_watchers`
before the sweep and drops the box
(`make_invalidation_function._invalidate_now`, quasiimmut.py:129-134),
so the next registration allocates a fresh instance.
Assisted-by: Claude
`w_type_notify_quasi_immut_watchers` read the raw `quasi_immut_watchers` pointer, nulled it and dropped the `Box` with no synchronisation, and `w_type_register_quasi_immut_watcher` allocated and pushed through the same unguarded pointer. Upstream's `_invalidate_now` (quasiimmut.py:129-134) is indivisible against `register_loop_token` because of the GIL; pyre runs Python threads on real OS threads (`module/thread/mod.rs` `start_new_thread`) and has none, so two threads mutating one class both saw the same instance and both freed it, and a mutator could free the instance a compiling thread was pushing into. The field becomes an `AtomicPtr` plus a `quasi_immut_lock`. The bare null test stays outside the lock; the get-or-create, every dereference and the `Box` free happen under it. The lock is per type rather than address-striped because a `W_TypeObject` is allocated through `try_gc_alloc_stable_raw` or `malloc_typed` and never moves. The sweep moves to a free function, `w_type_sweep_quasi_immut_watchers`, registered in `jit_fnaddr`: `dont_look_inside` emits a call target only for a free function, so the annotation on the `QuasiImmut::invalidate` method left the tracer with a policy marker and no trampoline. `type_object_destructor` now drops the instance of a swept type. `quasi_immut_publication_is_serialised_across_threads` aborts with heap corruption in `Vec` when the sweep is reverted to an unlocked load/store. Assisted-by: Claude
`W_TypeObject._version_tag?` and `ModuleDictStrategy.version?` are the tree's two `_immutable_fields_` entries spelled with a `?`. Upstream serves both from one `QuasiImmut` class reached through the hidden `mutate_<name>` pointer field the rtyper synthesises; pyre had a `QuasiImmut` in `typeobject` for the first and a bare `version_watchers: Vec` in `celldict` for the second. Both now hold a `quasiimmut::QuasiImmutField`, the synthesised field written out. For `version_watchers` that changes three things: - `register_version_watcher` pushed with no synchronisation while `mutated()` swept the same `Vec` from another thread. - `register_version_watcher` had no `compress_looptokens_list` (quasiimmut.py:77-82), so a module recompiled against many times and never mutated grew one entry per compile. - `mutated()` now unlinks the instance before sweeping it (`_invalidate_now`, quasiimmut.py:129-134) instead of retaining a swept list. `sweep_version_watchers` and `w_type_sweep_quasi_immut_watchers` collapse into one residual, `quasiimmut::sweep_quasi_immut_field`. check.py dynasm 358/358. Assisted-by: Claude
The global cell folds recorded `QUASIIMMUT_FIELD(dict, slot)` with no descr and a slot index standing in for one. Nothing could revalidate it: `OptHeap` gates the `is_still_valid_for` port (heap.py:794-808) on `op.getdescr()`, so the descr-less namespace form skipped it and a global rebound between the walker recording the fold and the optimizer running left the baked `ConstPtr(cell)` in place. The marker now names the field the dependency is actually on — `ModuleDictStrategy.version`, `celldict.py:34 _immutable_fields_ = ["version?"]`, the same field `getdictvalue_no_unwrapping` promotes before its elidable lookup. `_setitem_str_cell_known` (celldict.py:80-90) calls `mutated()` before every write that replaces a stored pointer and an in-place cell write leaves the pointer alone, so the version is what proves the baked address still stands. `record_namespace_quasiimmut_field` and the descr-less branch in `optimize_QUASIIMMUT_FIELD` are gone, and so is the `usize::MAX` slot sentinel the absent-name guard used to keep its key from colliding. The marker attaches to the strategy box, where `version` lives, so `register_quasi_immutable_deps` selects the registrar by the recorded field index: a `ModuleDictStrategy` has no `PyObject` header for the type-keyed one to read. The recorded `u32` therefore becomes the descr registry index rather than an effect index; nothing read it before. Two folds against one namespace now share one `QUASIIMMUT_FIELD` instead of carrying one per slot. `emit_namespace_cell_store_fold` returns whether it folded. It could not decline before; now that it can, an `Ok(())` would have told the caller the store was emitted when it was not. `register_quasi_immutable_deps` is the only drain of `last_quasi_immutable_deps`, and it ran only when the attempt produced a new artifact, so a recompile's dependencies stayed in the field and were registered against the next compile's flag. check.py dynasm 358/358, cranelift 358/358. `synth/global_quasiimmut_invalidation` prints 1000099 instead of 99505000 with the strategy registration disabled. Assisted-by: Claude
`JitDriver` spawned a thread that woke every 50 ms and called `QuasiImmut::invalidate()` on an `epoch_qmut` that every `run_back_edge` had registered every invalidation flag of the running loop token into. Flipping those flags fails `GUARD_NOT_INVALIDATED` in compiled code and makes `memmgr` evict the token (memmgr.py:70-73), on a wall-clock schedule. `rpython/jit/metainterp/quasiimmut.py` has no timer, thread or sleep, and no other RPython path invalidates a loop on a clock: `invalidate` is reached only from `_invalidate_now`, on the write to the quasi-immutable field. The comment claiming this stood in for "RPython's GC/signal- triggered invalidation" described a mechanism that does not exist upstream. `with_options`' `periodic_invalidation` opt-out had exactly one caller, `new`, passing `true`; no consumer ever declined it. That leaves `majit_metainterp::quasiimmut` with no users at all — its `get_current_qmut_instance` and `make_invalidation_function` already had none — so it goes with the timer. The live port is `pyre_object::quasiimmut`, which is where the two `?` fields' instances hang and which `pyre-object` can reach without depending on the metainterp. check.py dynasm 358/358, cranelift 358/358. Min-of-7 CPU time over int_loop / fib_loop / nested_loop / float_loop / fannkuch / nbody / spectral_norm moves between -1.8% and +3.0%. Assisted-by: Claude
`w_type_add_subclass` set the prebuilt-family dirty bit at the top of the function, before the `existing == w_subclass` early return that stores nothing, so a re-registration marked the whole prebuilt family dirty and cost the next minor collection a full walk for no store. Both store sites now call `note_weak_subclass_store`, which marks and then takes the write barrier the two arms previously duplicated. The mark stays ahead of the barrier. Host-side allocation cannot collect — `dynasm_alloc_nursery_typed` routes to `try_alloc_nursery_no_collect_typed` and spills to old-gen on nursery full — but `try_gc_write_barrier` reaches `gc_sync::gc_op`, which leaves RUNNING and parks on `gc_mutex`: an entry-style safepoint where another thread's stop-the-world collection runs. Marking behind it would expose a window where the slot is updated and the bit is clear. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
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
https://github.com/youknowone/pyre/blob/24f84e44c833de7d07a5d79cffc9d2f818b543c3/pyre-jit/src/eval.rs#L5299
When another Python thread mutates a quasi-immutable field after quasiimmut_field_still_valid rereads it but before this post-compilation registration runs, invalidation can remove the old QuasiImmut (or observe no installed instance), after which this loop creates/registers against a fresh instance even though the artifact baked the previous value. Its invalidation flag therefore remains false, and with the periodic invalidator removed the stale loop can execute indefinitely until an unrelated later mutation. Preserve the trace-time QuasiImmut identity/generation through registration, or synchronize the revalidation and publication with mutation as one protocol.
AGENTS.md reference: AGENTS.md:L194-L196
ℹ️ 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".
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 24f84e4). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
This was referenced Aug 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Follow-up to #940, which made
W_TypeObject._version_tagquasi-immutable. Reviewingthat merge surfaced that the tracking it relies on was keyed on the wrong structure,
that nothing revalidated a
?field between tracing and optimizing, and that abackground thread was invalidating every compiled loop on a wall clock.
quasiimmut_seenwas a faithful port with zero usersCacheEntry.quasiimmut_seen{,_refs}(heapcache.rs:78-79) is cleared exactly whereheapcache.py:70-77/:121-129clears it — and nothing read it. A pyre-onlyHeapCache.quasi_immut_knownflat set, cleared only byreset()at trace teardown,did the work instead.
clear_caches_varargs(heapcache.py:341-370) was alreadyarming
need_guard_not_invalidatedper general call; it just had nothing to clear,so the second folded lookup after a residual call reused the first one's pin and no
second
GUARD_NOT_INVALIDATEDwas emitted (heap.py:810-822).Two un-inlined method calls on one instance:
QuasiimmutField×1 becomes ×2, eachfollowed by
GuardNotInvalidated(), with the call between. The method-cache pair'ssteady body goes 38 → 39 ops — the 38 in #940's title was unsound, so #940's
"41 → 38" reads "41 → 39" after this branch.
QuasiImmutDescr.is_still_valid_forhad no port, and the namespace form had no descrquasiimmut.py:135-143capturesconstantfieldboxat trace time andheap.py:794-808raises
InvalidLoop('quasi immutable field changed during tracing')when the fieldmoved before optimization. Pyre had neither half.
The captured value now rides on the op as
arg(1)(TraceCtx::field_sanity_load, thebh_getfield_gc_*triple) andOptHeap::quasiimmut_field_still_validre-reads throughOptContext::get_runtime_field. A per-op descr like upstream's is not available here:pyre descrs are registry-indexed singletons, so a fresh
Arcper read mints a registryentry per read and misses
heap.rs:3274 quasi_immut_cache, which keys on theArcpointer.
The module-global folds were worse off: they recorded
QUASIIMMUT_FIELD(dict, slot)with no descr at all, and
optimize_QUASIIMMUT_FIELDgates the revalidation onop.getdescr(), so that form skipped it entirely. The marker now names the field thedependency is really on —
ModuleDictStrategy.version,celldict.py:34 _immutable_fields_ = ["version?"]— the same fieldgetdictvalue_no_unwrappingpromotes before its elidable lookup.
_setitem_str_cell_known(celldict.py:80-90)calls
mutated()before every write that replaces a stored pointer, and an in-placecell write leaves the pointer alone, so the version is exactly what proves the baked
ConstPtr(cell)still stands.Two folds against one namespace now share one
QUASIIMMUT_FIELDwhere the per-slotscheme emitted one each.
One
QuasiImmut, and it is not thread-safe by inheritancerpython/jit/metainterp/quasiimmut.pyis now ported once, inpyre-object/src/quasiimmut.rs, and both?fields share it —W_TypeObject._version_tag(typeobject.py:177) andModuleDictStrategy.version.Before this each had its own bare
Vec<Weak<..>>: nocompress_looptokens_list, so anobject recompiled against many times and never mutated grew one entry per compile, and
ModuleDictStrategy's was pushed to without synchronisation whilemutated()swept itfrom another thread.
rg 'allocate_lock|Lock\(' rpython/jit/returns nothing: upstream gets thenull-then-sweep pair of
_invalidate_now(quasiimmut.py:129-134) free from the GIL.pyre runs Python threads on real OS threads, so
QuasiImmutFieldcarries anAtomicPtrplus aparking_lot::Mutex<()>, swapping under the lock. Only thedereferencing paths take it —
is_installed()stays lock-free, because that bare testis the whole cost a mutation on an object no loop watches has to pay. Both owners are
allocated non-moving (
try_gc_alloc_stable_raw/malloc_typed), so an embedded lockcannot be remapped out from under a holder.
The stress test was written against a deliberately-unlocked control first: two
registrar threads spinning while four mutators each run 200 000 invalidations aborts
the control with heap corruption, and passes with the lock.
The 50 ms loop-invalidation timer has no upstream counterpart
JitDriverspawned a thread that woke every 50 ms and invalidated anepoch_qmutthatevery back edge had registered the running loop token's invalidation flags into.
Flipping those fails
GUARD_NOT_INVALIDATEDin compiled code and makesmemmgrevictthe token (
memmgr.py:70-73), on a wall-clock schedule.quasiimmut.pyhas no timer,thread or sleep, and
invalidateis reached only from_invalidate_now, on the writeto the field. The comment claiming this stood in for "RPython's GC/signal-triggered
invalidation" described a mechanism that does not exist upstream, and
with_options' opt-out had one caller —new, passingtrue.It also self-healed every stale const-fold within 50 ms, which is what made a
missing-invalidation bug unreproducible. Removing it leaves
majit_metainterp::quasiimmutwith no users at all (get_current_qmut_instanceandmake_invalidation_functionalready had none), so that module goes too.w_type_add_subclass's dirty markUnrelated to the
?fields, found while auditing the same file. The prebuilt-familydirty bit was set at the top of
w_type_add_subclass, ahead of theexisting == w_subclassearly return that stores nothing, so a re-registrationdirtied the whole prebuilt family and cost the next minor collection a full walk for
no store. Both store sites now call one helper that marks and then takes the write
barrier the two arms duplicated.
The mark stays ahead of the barrier. Host-side allocation cannot collect —
dynasm_alloc_nursery_typedroutes totry_alloc_nursery_no_collect_typedand spillsto old-gen on nursery full — but
try_gc_write_barrierreachesgc_sync::gc_op,which leaves RUNNING and parks on
gc_mutex: an entry-style safepoint where anotherthread's stop-the-world collection runs. Marking behind it would leave a window with
the slot updated and the bit clear. The other 21
mark_prebuilt_roots_dirtycallsites were audited against that rule and none violate it.
Verification
On
e6e2b25a715(#930), the current base:check.py --backend dynasm,cranelift— 358/358 each, 2/2 backend runscargo test --release -p pyre-object -p pyre-jit-trace -p majit-metainterp -p majit-trace— 2207 passed, 0 failedcargo test --release -p pyre-jit --features dynasm --test gc_stress— 23/23cargo fmt --all --checkcleanThe perf figure was taken before the rebase, on
a9a867e81ee: min-of-7 CPU time overint_loop / fib_loop / nested_loop / float_loop / fannkuch / nbody / spectral_norm moves
between −1.8% and +3.0% with the timer gone. The branch delta is byte-identical across
the two bases, so it was not re-measured.
— authored by Claude