-
Notifications
You must be signed in to change notification settings - Fork 19
gc: the young raw-malloced generation, and the max-heap MemoryError the dispatch loop owes #1474
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 12 commits
2ebd9f9
8961c9b
3080402
18b101d
76dbcdc
3925826
4057a42
3f4bd3d
1826f73
4bd356d
361c860
7ca707d
5174d90
09e2cd9
7111bf7
64cc79b
1bee8a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,12 @@ | |
| //! bit4 EB_GC — the old-gen allocator reached the next-major threshold; | ||
| //! OR'd in by the collector, consumed by the interpreter | ||
| //! dispatch-loop GC safepoint. | ||
| //! bit5 EB_MEMORY_ERROR — a bounded major collection exhausted the heap; | ||
| //! OR'd in by the collector where upstream raises, consumed | ||
| //! by the dispatch loop, which raises `MemoryError` there. | ||
| //! Published for every thread because it is a deopt trigger, | ||
| //! but delivered only to the thread whose collection | ||
| //! exhausted the heap; see `set_memory_error`. | ||
| //! A compiled loop loads the whole word at the back-edge and deopts to the | ||
| //! interpreter when it is non-zero. The interpreter/warm-up loop and the STW | ||
| //! park gate remain authoritative; this word is only the JIT's deopt trigger. | ||
|
|
@@ -28,6 +34,7 @@ | |
| //! can briefly deopt before the request is visible, then resume and re-deopt | ||
| //! until coherence propagates it; this is a bounded park-latency window. | ||
|
|
||
| use std::cell::Cell; | ||
| use std::sync::atomic::{AtomicUsize, Ordering}; | ||
|
|
||
| /// bit0 — async action / signal pending (mirrors a negative ticker). | ||
|
|
@@ -41,12 +48,19 @@ pub const EB_GC_INTERP: usize = 8; | |
| /// bit4 — the old-gen allocator crossed the next-major threshold and a | ||
| /// collection is owed at the next root-complete point. | ||
| pub const EB_GC: usize = 16; | ||
| /// bit5 — a bounded major collection reached `max_heap_size` and a | ||
| /// `MemoryError` is owed at the next root-complete point. | ||
| pub const EB_MEMORY_ERROR: usize = 32; | ||
| /// Bits that require a compiled loop to deopt to the interpreter. | ||
| /// | ||
| /// `EB_GC` belongs here: the allocator only arms the request, and the | ||
| /// collection itself runs at the dispatch-loop safepoint, so a compiled loop | ||
| /// has to leave machine code for the request to be serviced at all. | ||
| pub const JIT_BREAKER_MASK: usize = EB_ASYNC | EB_STW | EB_FINALIZING | EB_GC; | ||
| /// | ||
| /// `EB_MEMORY_ERROR` belongs here for the same reason: the exception is raised | ||
| /// by the dispatch loop, so a compiled loop that never returns to it would run | ||
| /// on past a heap the collector has already declared exhausted. | ||
| pub const JIT_BREAKER_MASK: usize = EB_ASYNC | EB_STW | EB_FINALIZING | EB_GC | EB_MEMORY_ERROR; | ||
|
|
||
| /// The shared eval-breaker word (see module docs). | ||
| static EVAL_BREAKER_WORD: AtomicUsize = AtomicUsize::new(0); | ||
|
|
@@ -137,6 +151,110 @@ pub fn take_gc() -> bool { | |
| EVAL_BREAKER_WORD.fetch_and(!EB_GC, Ordering::Relaxed) & EB_GC != 0 | ||
| } | ||
|
|
||
| // --- memory error (bit5): armed by the collector, raised by the dispatch loop --- | ||
|
|
||
| thread_local! { | ||
| /// Whether *this* thread armed a `MemoryError` its own dispatch loop has | ||
| /// not raised yet. | ||
| /// | ||
| /// The exception belongs to the thread whose collection exhausted the | ||
| /// heap, the way upstream's `raise MemoryError` unwinds through whichever | ||
| /// driver was on the stack. The bit alone cannot say that: it is process | ||
| /// global, and a stop-the-world collection resumes the other mutators | ||
| /// before the collecting one returns to its dispatch loop, so without this | ||
| /// an unrelated thread could reach a back edge first and take an exception | ||
| /// nothing it did earned — leaving the thread that did exhaust the heap | ||
| /// running on toward the fatal rung. | ||
| static MEMORY_ERROR_OWED: Cell<bool> = const { Cell::new(false) }; | ||
| } | ||
|
|
||
| /// How many threads owe an undelivered `MemoryError`. | ||
| /// | ||
| /// The bit is the summary a back edge polls, and only the last owner to | ||
| /// deliver may clear it. Counting is what tells that owner apart from the | ||
| /// others. | ||
| static MEMORY_ERROR_OWERS: AtomicUsize = AtomicUsize::new(0); | ||
|
Comment on lines
+228
to
+229
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When thread A owns a deferred error and thread B forks during the post-STW/pre-dispatch window, the child inherits AGENTS.md reference: AGENTS.md:L152-L154 Useful? React with 👍 / 👎. |
||
|
|
||
| /// Record that a bounded major collection reached `max_heap_size`. | ||
| /// | ||
| /// incminimark.py `major_collection_step` reacts to that with a plain `raise | ||
| /// MemoryError`, so upstream's exception surfaces wherever the collection was | ||
| /// driven from. Most of pyre's collections are driven from the dispatch-loop | ||
| /// safepoint, which returns `()` and cannot raise, so the collector arms this | ||
| /// bit and the loop raises on the next dispatch instead. The bit rather than a | ||
| /// return value, because the safepoint is one of several drivers — an | ||
| /// allocation, a finalizer run and an explicit collection request reach the | ||
| /// same collection — and only the dispatch loop can turn any of them into an | ||
| /// exception. | ||
| /// | ||
| /// Call this on the thread that drove the collection: the exception is owed to | ||
| /// that thread, and the bit only publishes that one is owed to someone. | ||
| pub fn set_memory_error() { | ||
| MEMORY_ERROR_OWED.with(|owed| { | ||
| if owed.replace(true) { | ||
| // Already owed here and not yet delivered, so the count and the | ||
| // bit already stand for this thread. | ||
| return; | ||
| } | ||
| MEMORY_ERROR_OWERS.fetch_add(1, Ordering::AcqRel); | ||
| EVAL_BREAKER_WORD.fetch_or(EB_MEMORY_ERROR, Ordering::Relaxed); | ||
| }); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| /// Whether a `MemoryError` armed by [`set_memory_error`] is still owed, to any | ||
| /// thread. | ||
| /// | ||
| /// [`take_memory_error`] clears the bit as the last owner delivers, so this | ||
| /// reads true for exactly the window between arming an exception and every | ||
| /// thread that was owed one raising it. The collector reads it to tell a breach | ||
| /// that arrives inside that window from one that arrives after the program has | ||
| /// had its exception — a question about the program, not about a thread, which | ||
| /// is why this reads the process-wide summary. | ||
| pub fn memory_error_armed() -> bool { | ||
| EVAL_BREAKER_WORD.load(Ordering::Relaxed) & EB_MEMORY_ERROR != 0 | ||
| } | ||
|
|
||
| /// Whether *this* thread is the one still owed a `MemoryError`. | ||
| /// | ||
| /// The collector's max-heap ladder asks it to tell a breach that repeats an | ||
| /// exception this thread has already been given from one that is new to it. | ||
| /// [`memory_error_armed`] cannot answer that: it reports the process-wide | ||
| /// summary, so an exception owed to another thread would read as this thread's | ||
| /// own and silence a breach that thread never caused. | ||
| pub fn memory_error_owed_here() -> bool { | ||
| MEMORY_ERROR_OWED.with(|owed| owed.get()) | ||
| } | ||
|
|
||
| /// Consume the `MemoryError` owed to *this* thread, reporting whether one was. | ||
| /// | ||
| /// A thread that is not owed one leaves the bit alone and deopts again at its | ||
| /// next back edge, until the owner delivers. That window is bounded the way the | ||
| /// STW one is: arming happens inside the owner's own safepoint, so its very | ||
| /// next dispatch takes it. | ||
| pub fn take_memory_error() -> bool { | ||
| // Same shape as `take_gc`: the taker runs per dispatch and the armer only | ||
| // when a bounded heap is exhausted, so keep the common case a plain load. | ||
| if EVAL_BREAKER_WORD.load(Ordering::Relaxed) & EB_MEMORY_ERROR == 0 { | ||
| return false; | ||
| } | ||
| MEMORY_ERROR_OWED.with(|owed| { | ||
| if !owed.replace(false) { | ||
| return false; | ||
| } | ||
| if MEMORY_ERROR_OWERS.fetch_sub(1, Ordering::AcqRel) == 1 { | ||
| EVAL_BREAKER_WORD.fetch_and(!EB_MEMORY_ERROR, Ordering::Relaxed); | ||
| // Another thread may have armed one between the decrement and the | ||
| // clear, and set a bit this then removed. Re-publish the summary it | ||
| // is owed; setting an already-set bit is what the racing arm did | ||
| // anyway, so this is idempotent rather than a second signal. | ||
| if MEMORY_ERROR_OWERS.load(Ordering::Acquire) != 0 { | ||
| EVAL_BREAKER_WORD.fetch_or(EB_MEMORY_ERROR, Ordering::Relaxed); | ||
| } | ||
| } | ||
| true | ||
| }) | ||
| } | ||
|
|
||
| /// Depth of the operation chain between the poll's load and its guard. | ||
| /// | ||
| /// The recorder emits `RawLoadI -> IntAnd -> IntIsTrue -> GuardFalse`, so two | ||
|
|
@@ -206,6 +324,87 @@ mod tests { | |
| crate::operand::Operand::from_bound_op(&Rc::new(op)) | ||
| } | ||
|
|
||
| /// Both memory-error tests read the process-global bit and assert on its | ||
| /// resting state, so they cannot run at the same time as each other. | ||
| static MEMORY_ERROR_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); | ||
|
|
||
| /// The owed `MemoryError` is one-shot: armed once by the collector, raised | ||
| /// once by the dispatch loop of the thread it is owed to. A second take | ||
| /// must see nothing, or one breach would raise two `MemoryError`s. | ||
| /// | ||
| /// It must also be a bit the back-edge poll tests, since the raise happens | ||
| /// in the dispatch loop and a compiled loop has to leave machine code to | ||
| /// get there. | ||
| #[test] | ||
| fn the_owed_memory_error_is_taken_exactly_once() { | ||
| let _guard = MEMORY_ERROR_TEST_LOCK | ||
| .lock() | ||
| .unwrap_or_else(|e| e.into_inner()); | ||
| assert!( | ||
| !take_memory_error(), | ||
| "nothing has armed the bit, so it must not report one pending" | ||
| ); | ||
| set_memory_error(); | ||
| assert_ne!( | ||
| load() & JIT_BREAKER_MASK, | ||
| 0, | ||
| "an armed MemoryError must fail the back-edge poll" | ||
| ); | ||
| assert!(take_memory_error(), "the armed bit is reported once"); | ||
| assert!(!take_memory_error(), "and only once"); | ||
| assert_eq!( | ||
| load() & EB_MEMORY_ERROR, | ||
| 0, | ||
| "taking it clears it, so later back edges are not deopted" | ||
| ); | ||
| } | ||
|
|
||
| /// The exception belongs to the thread whose collection exhausted the | ||
| /// heap, not to whichever dispatch loop polls first. | ||
| /// | ||
| /// Upstream raises inside the collection, so the driver on that thread's | ||
| /// stack is the one that receives it. Pyre defers through a process-global | ||
| /// word, and a stop-the-world collection resumes the other mutators before | ||
| /// the collecting one returns to its own dispatch loop — so without | ||
| /// ownership the first unrelated thread to reach a back edge would take an | ||
| /// exception nothing it did earned, and the thread that did exhaust the | ||
| /// heap would run on toward the fatal rung with nothing owed to it. | ||
| #[test] | ||
| fn the_owed_memory_error_goes_to_the_thread_that_armed_it() { | ||
| let _guard = MEMORY_ERROR_TEST_LOCK | ||
| .lock() | ||
| .unwrap_or_else(|e| e.into_inner()); | ||
| let owner = std::thread::spawn(|| { | ||
| set_memory_error(); | ||
| // The summary is published for everyone, so a compiled loop on any | ||
| // thread leaves machine code — the bit is the deopt trigger, not | ||
| // the delivery. | ||
| assert_ne!(load() & EB_MEMORY_ERROR, 0); | ||
| assert!(memory_error_armed()); | ||
| }); | ||
| owner.join().unwrap(); | ||
|
|
||
| assert!( | ||
| memory_error_armed(), | ||
| "the exception is still owed, so the summary stays published" | ||
| ); | ||
| assert!( | ||
| !take_memory_error(), | ||
| "this thread armed nothing, so it is owed nothing" | ||
| ); | ||
| assert_ne!( | ||
| load() & EB_MEMORY_ERROR, | ||
| 0, | ||
| "and a thread that is owed nothing must not clear the owner's bit" | ||
| ); | ||
|
|
||
| // The owner is gone without delivering, which is what leaves the bit | ||
| // set here; clear it so the resting state is what the next test finds. | ||
| MEMORY_ERROR_OWED.with(|owed| owed.set(true)); | ||
| assert!(take_memory_error()); | ||
| assert_eq!(load() & EB_MEMORY_ERROR, 0); | ||
| } | ||
|
|
||
| fn int(value: i64) -> crate::operand::Operand { | ||
| crate::operand::Operand::const_(Const::Int(value)) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -232,7 +232,7 @@ its frames on the heap. Lowering it is how the `SubWalkDepthExceeded` decline | |
| is exercised without building a pathological helper chain; it retires when the | ||
| descent stops recursing on the host stack. | ||
|
|
||
| ### §6c — Default-OFF diagnostics, censuses and probes (72): keep, cost nothing | ||
| ### §6c — Default-OFF diagnostics, censuses and probes (73): keep, cost nothing | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Update the §6c count to 74. The list contains 74 distinct entries, but the heading still says Proposed fix-### §6c — Default-OFF diagnostics, censuses and probes (73): keep, cost nothing
+### §6c — Default-OFF diagnostics, censuses and probes (74): keep, cost nothingAlso applies to: 253-254 🤖 Prompt for AI Agents |
||
|
|
||
| Each is inert unless set, so none is a removal target by this file's | ||
| already-ON criterion. They are listed so they cannot be missed again. | ||
|
|
@@ -250,7 +250,8 @@ already-ON criterion. They are listed so they cannot be missed again. | |
| `PYRE_FBW_STRICT_DIAG`, | ||
| `PYRE_FIELD_IDENTITY_CENSUS`, | ||
| `PYRE_FORITER_INFLIGHT_CENSUS`, `PYRE_FOR_ITER_GATE_DIAG`, | ||
| `PYRE_GC_DIAG`, `MAJIT_GC_FREELIST_DIAG`, `PYRE_GEN_ENTRY_DIAG`, | ||
| `PYRE_GC_DIAG`, `MAJIT_GC_FREELIST_DIAG`, `PYRE_GC_SIZE_AUDIT`, | ||
| `PYRE_GEN_ENTRY_DIAG`, | ||
| `PYRE_JD1_DEBUG`, `PYRE_JD1_DUMP`, | ||
| `PYRE_LB_SITE`, `PYRE_LLBC_SKIP_FINGERPRINT_CHECK`, `PYRE_LLBC_STRICT`, | ||
| `PYRE_LOOP_CENSUS`, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2271,6 +2271,32 @@ fn eval_loop(frame: &mut PyFrame) -> PyResult { | |
| if dispatch_breaker & majit_ir::eval_breaker_word::EB_STW != 0 { | ||
| majit_gc::gc_sync::safepoint_poll(); | ||
| } | ||
| // A bounded major collection that reached `max_heap_size` owes a | ||
| // `MemoryError` — incminimark.py `major_collection_step` raises one | ||
| // there, and the safepoint above is where the interpreter path's | ||
| // collections happen but it returns `()` and cannot raise. Deliver it | ||
| // here, at the same seam an asynchronously delivered signal uses below: | ||
| // the block search runs at `last_instr`, so a `try` around the region | ||
| // that was running catches it rather than the frame unwinding. | ||
| // | ||
| // Reads the word rather than `dispatch_breaker`: the safepoint above is | ||
| // the usual armer and it runs after that load, so testing the loaded | ||
| // copy would defer delivery by a dispatch this loop is not guaranteed | ||
| // to reach. `take_memory_error` opens with a relaxed load and returns | ||
| // on it, so the ordinary dispatch pays that load and a branch against a | ||
| // word it just touched. | ||
| // | ||
| // After the park above, not before it: `handle_exception` runs Python | ||
| // and allocates, and both bits can be armed at once — this thread's | ||
| // own breach and another thread's STW request. Delivering first would | ||
| // run a handler through a world the collector has asked to stop. | ||
| if majit_ir::eval_breaker_word::take_memory_error() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If another thread arms AGENTS.md reference: AGENTS.md:L152-L154 Useful? React with 👍 / 👎. |
||
| let mut err = crate::PyError::memory_error(""); | ||
| if handle_exception(frame, &mut err, &mut next_instr) { | ||
|
Comment on lines
+2293
to
+2301
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When another thread requests STW after this thread's heap-limit breach but before its next plain-evaluator dispatch, both breaker bits can be set. This branch consumes the error and enters AGENTS.md reference: AGENTS.md:L152-L154 Useful? React with 👍 / 👎. |
||
| continue; | ||
| } | ||
| return Err(err); | ||
| } | ||
|
|
||
| if next_instr >= code.instructions.len() { | ||
| return Ok(w_none()); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9064,6 +9064,37 @@ fn eval_loop_jit(frame: &mut PyFrame) -> LoopResult { | |
| // maybe_compile_and_run, jit_merge_point). | ||
| let f: *mut PyFrame = frame_root.frame() as *mut PyFrame; | ||
|
|
||
| // The plain evaluator's twin: a bounded major collection that reached | ||
| // `max_heap_size` owes a `MemoryError`, and the safepoint above — which | ||
| // is where the interpreter path collects — cannot raise one. | ||
| // `EB_MEMORY_ERROR` is in `JIT_BREAKER_MASK`, so a compiled loop leaves | ||
| // machine code for this seam instead of running on past an exhausted | ||
| // heap, and delivering before `jit_merge_point` below keeps a hot loop | ||
| // from re-entering compiled code with the exception still owed. | ||
| // | ||
| // Reads the word rather than `dispatch_breaker`, and the difference is | ||
| // not academic: the safepoint above is the usual armer and it runs | ||
| // after that load, so `dispatch_breaker` is one dispatch stale. This | ||
| // loop is not guaranteed a next dispatch — measured, a run under | ||
| // `--heapsize` reaches this seam with the bit set exactly once — so a | ||
| // stale test does not delay the exception, it drops it, and the | ||
| // program's next sign of a full heap is the abort on the second breach. | ||
| if majit_ir::eval_breaker_word::take_memory_error() { | ||
| let mut err = pyre_interpreter::PyError::memory_error(""); | ||
| let mut next_instr = unsafe { &*f }.next_instr(); | ||
| if pyre_interpreter::eval::handle_exception( | ||
| unsafe { &mut *f }, | ||
| &mut err, | ||
| &mut next_instr, | ||
| ) { | ||
| // handle_exception allocates → re-seed before the write. | ||
| let f: *mut PyFrame = frame_root.frame() as *mut PyFrame; | ||
| unsafe { &mut *f }.set_last_instr_from_next_instr(next_instr); | ||
| continue; | ||
| } | ||
| return LoopResult::Done(Err(err)); | ||
| } | ||
|
Comment on lines
+9067
to
+9102
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- changed files ---'
git diff --stat
printf '%s\n' '--- relevant diff ---'
git diff -- pyre/pyre-jit/src/eval.rs | sed -n '1,220p'
printf '%s\n' '--- prepass and ullbc references ---'
rg -n --hidden -S 'build/llbc|ullbc|prepass|PYRE_RTYPER_VERBOSE|re-extract|extract' \
-g '!build/**' -g '!target/**' . | sed -n '1,260p'Repository: youknowone/pyre Length of output: 25696 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- candidate build and workflow files ---'
git ls-files | rg '(^|/)(build|scripts|tools|\.github|Makefile|justfile|Cargo\.toml|README|CONTRIBUTING)' | sed -n '1,260p'
printf '%s\n' '--- Rust file context ---'
sed -n '8870,8985p' pyre/pyre-jit/src/eval.rsRepository: youknowone/pyre Length of output: 13418 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- repository guidance ---'
sed -n '35,80p' AGENTS.md
printf '%s\n' '--- extraction driver ---'
sed -n '1,180p' scripts/extract-llbc.py
printf '%s\n' '--- prepass manifest and build instructions ---'
sed -n '1,90p' pyre/pyre-jit-trace/Cargo.toml
sed -n '1,90p' pyre/pyre-jit/Cargo.toml
printf '%s\n' '--- working-tree and artifact state ---'
git status --short
find build/llbc -maxdepth 2 -type f \( -name '*.ullbc' -o -name '*.fingerprint' \) -printf '%p\n' 2>/dev/null | sort | sed -n '1,120p'Repository: youknowone/pyre Length of output: 10271 Re-extract Because 🤖 Prompt for AI AgentsSource: Path instructions |
||
|
|
||
| if unsafe { &*f }.next_instr() >= code.instructions.len() { | ||
| return LoopResult::Done(Ok(w_none())); | ||
| } | ||
|
|
||
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.
If a host or background thread drives the public collection API under a heap limit and exits before entering another evaluator dispatch, this plain
Cell<bool>is destroyed without decrementingMEMORY_ERROR_OWERS. The new ownership test demonstrates the state by letting the owner terminate and then manually impersonating it to clean up; production has no equivalent cleanup. The process-wideEB_MEMORY_ERRORtherefore remains armed permanently, causing compiled backedges to keep deoptimizing and preventingmemory_error_armed()from releasing the fatal heap-limit rung. Give the TLS value a thread-exit destructor or tie ownership cleanup to the mutator lifecycle.Useful? React with 👍 / 👎.