jit: #956's quasi-immut revalidation uses a value comparison where upstream tests qmut identity — InvalidLoop storm, 3.7x slower, main is red

[youknowone]

origin/main (f3bddb31f28) fails pyre/check.py --backend dynasm:

FAIL dynasm synth/exception_reraise_tb_depth_jitstress
     jit-stats regression: loops_aborted 0 -> 1198, loops_compiled 805 -> 305

Bisected to #956 (28880890a7e). It is a real slowdown, not a shape change.

Bisect

Every point rebuilt with a full LLBC regeneration (python3 pyre/scripts/extract-llbc.py), since #956 touches pyre-jit / pyre-interpreter / pyre-object.

commit	loops_compiled	bridges_compiled	loops_aborted	guard_failures
e6e2b25a715 (#930) — #956's parent	805	1	0	1798
28880890a7e (#956)	305	0	1198	1199

A/B

Interleaved, alternating binaries, min-of-5, user+sys CPU, same host, exception_reraise_tb_depth_jitstress:

build	CPU
e6e2b25a715	0.380s
28880890a7e	1.410s

3.7x slower. The _hot twin is unaffected (4 / 9 / 0 / 1803 on both): the regression only appears under pypyjit.set_param("threshold=1,function_threshold=1"), i.e. when tracing is aggressive.

Mechanism

#956 added OptHeap::quasiimmut_field_still_valid (majit/majit-metainterp/src/optimizeopt/heap.rs), which gates InvalidLoop("quasi immutable field changed during tracing") at the QuasiimmutField arm.

The heap.py:794-796 precondition is ported correctly — a struct that did not fold to a constant returns "still valid". The divergence is the test itself.

Upstream rpython/jit/metainterp/quasiimmut.py:146-158:

def is_still_valid_for(self, structconst):
    assert self.struct
    if self.struct != structconst.getref_base():
        return False
    cpu = self.cpu
    qmut = get_current_qmut_instance(cpu, self.struct, self.mutatefielddescr)
    if qmut is not self.qmut:
        return False
    else:
        currentbox = self.get_current_constant_fieldvalue()
        assert self.constantfieldbox.same_constant(currentbox)   # <- an assert
        return True

The decision is instance identity — invalidation nulls the hidden mutate_* field, so get_current_qmut_instance hands back a fresh object and is not fires. The field-value comparison is only the assertion backing that up, and it is unreachable when the identity test already returned False.

pyre has no per-read QuasiImmutDescr to hang that identity on, so #956 substitutes the value comparison for the test — its own doc comment states this. That is strictly stronger than upstream: it reports "changed" whenever the value merely differs, not only when an invalidation actually occurred.

Consequence chain (measured)

Splitting ct_entry_bridge_failed — which today counts five unrelated compile_entry_bridge exits as one number — attributes it exactly:

• ceb_entered = 2396, ceb_invalidloop = 2396, all other exits 0

So every interpreter entry bridge is discarded in optimize_bridge, before any backend is involved. With no front door into the compiled loop, the interpreter re-reaches the back-edge decision every iteration (caro_backedge 1202 vs 6 on the parent), and compile_loop gives the retrace up at its own has_compiled_targets (pyjitpl.rs:5576-5591, upstream pyjitpl.py:3185-3189) → cl_hct_giveup = loops_aborted = 1198 ABORT_BAD_LOOP.

The abort is orthodox; the missing entry bridge is the defect.

Why no CI caught it

A semantic merge conflict. #947 recorded the 805/0 baseline against e6e2b25a715, where it was correct. #956 and #929 merged between #947's CI run and #947's merge:

git merge-base --is-ancestor 28880890a7e a115ce65a6e   # CI-tested branch tip -> NO
git merge-base --is-ancestor 28880890a7e 44e0c9ccb9a   # #947 merge on main  -> YES

GitHub validates the PR merge ref at PR time, not at merge time, so the combination was never built by anyone. Both PRs were individually green.

Reproduce

git checkout f3bddb31f28
python3 pyre/scripts/extract-llbc.py
cargo build --release -p pyrex --bin pyre-dynasm --no-default-features --features dynasm
MAJIT_STATS=1 ./target/release/pyre-dynasm pyre/bench/synth/exception_reraise_tb_depth_jitstress.py 2>&1 | grep loops_compiled

Expected loops_compiled=805 … loops_aborted=0; actual loops_compiled=305 … loops_aborted=1198.

— reported by Claude

[youknowone]

The identity test is portable — pyre already has the object upstream compares

I assumed at first that pyre simply lacks upstream's qmut identity and that #956 had no choice. That is not the case.

pyre/pyre-object/src/quasiimmut.rs defines QuasiImmutField, which its own module doc calls "upstream's hidden mutate_<name> field written out". It holds an AtomicPtr<QuasiImmut>, and the semantics line up exactly with upstream:

upstream	pyre
get_current_qmut_instance — read the hidden field, create if null	QuasiImmutField::register_loop_token — if qmut_ptr.is_null() { … store fresh … }
_invalidate_now nulls the hidden field	QuasiImmutField::invalidate → take() swaps the pointer to null
a later read creates a fresh instance ⇒ qmut is not self.qmut	a later register_loop_token allocates a fresh Box ⇒ different pointer

So QuasiImmutField.ptr is the identity is_still_valid_for compares. Invalidation is the only thing that nulls it, so the pointer changes if and only if a real invalidation happened in between — which is precisely the property #956's value comparison is standing in for, and does not have.

Concretely, the two live ? fields are named fields on real objects: W_TypeObject.quasi_immut_watchers (typeobject.rs:234) and ModuleDictStrategy.version_watchers (celldict.rs:644).

Why the value comparison is strictly stronger than upstream

In upstream the field-value comparison sits after the identity test, as an assert:

if qmut is not self.qmut:
    return False
else:
    currentbox = self.get_current_constant_fieldvalue()
    assert self.constantfieldbox.same_constant(currentbox)
    return True

RPython disables asserts in translated code, so once the identity matches upstream returns True regardless of the value. pyre returns False there, which is where the 2396 spurious InvalidLoops come from.

Suggested shape

The resolution from (object, field_index) to the right QuasiImmutField already exists — register_quasi_immutable_deps (pyre/pyre-jit/src/eval.rs:5366) dispatches on field_index == module_dict_version_descr().index() to either module_dict_strategy_register_version_watcher or w_type_register_quasi_immut_watcher. It is a write; what is needed is the matching read.

1. pyre-object — expose the identity next to the existing registrars: read QuasiImmutField.ptr, creating the instance when null so it matches get_current_qmut_instance (a trace-time read that does not create would later look like a change the first time any loop registers).
2. pyre-jit — register a hook that dispatches on field_index exactly as register_quasi_immutable_deps does.
3. majit-metainterp — reach it through an OptContext accessor, the same way 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 reaches the field value via get_runtime_field. This keeps the layering: upstream itself goes through cpu.bh_getfield_gc_r(gcref, mutatefielddescr), so a CPU/context-level accessor is the orthodox shape and majit-metainterp never imports pyre-object.
4. trace time (state.rs:4429 record_quasiimmut_field) — capture the identity where current_quasiimmut_field_value captures the value today, and ride it on the op the same way (the existing comment explains why it rides on the op rather than a per-read descr).
5. optimize time (heap.rs quasiimmut_field_still_valid) — compare identities. Keep the heap.py:794-796 non-constant early-out, which is already correct.

Not landing it here

I am not submitting this as a patch in the same pass that reported it: it spans three crates, changes a soundness check, and needs a full pyre/check.py run across all three backends to be trustworthy. Filing the design rather than a half-verified patch.

Two smaller notes for whoever picks it up:

• Do not "fix" the missing is_constant precondition — it is already implemented inside quasiimmut_field_still_valid.
• A blunt revert of just this check restores 805 / 0 and the 3.7x, but leaves pyre with neither upstream's identity test nor the value test. That is a rollback, not a fix.

Blast radius

Sweeping the synthetic corpus with both binaries (e6e2b25a715 vs 28880890a7e), comparing the full [jit-stats] counter line per fixture: only exception_reraise_tb_depth_jitstress differs. The sweep was stopped before the tail of the corpus, so this is a lower bound, not a complete census — but it covered the large majority and every fixture scanned produced a stats line (no silently-empty comparisons).

— commented by Claude