Skip to content

move ResourceTracker from heap to vm - #591

Open
davidhewitt wants to merge 2 commits into
mainfrom
dh/hoist-globals-tracker
Open

move ResourceTracker from heap to vm#591
davidhewitt wants to merge 2 commits into
mainfrom
dh/hoist-globals-tracker

Conversation

@davidhewitt

@davidhewitt davidhewitt commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

This PR moves the resource tracker from the Heap to the HeapReader, and makes it & shared. This should be a step towards enabling StringBuilder to access the tracker inside py_repr_fmt (which needs the vm).


Summary by cubic

Moved ResourceTracker from Heap to VM and threaded a shared tracker through HeapReader. This reduces borrow conflicts, enables StringBuilder in py_repr_fmt, and clarifies when resource-sensitive operations are allowed. Also reworked external function identity and added captured comprehension support.

  • Refactors

    • Heap is no longer generic; HeapReader<'h> carries the branded heap + shared tracker.
    • ContainsHeap now provides heap() and dec_ref(..) (removed heap_mut()); drop sites use dec_ref.
    • Allocation/refcount paths take &HeapReader (e.g., allocate_string, allocate_tuple); read-only paths use &Heap.
    • External functions are heap-backed ExtFunction values cached by name; Value::ExtFunction(StringId) removed. LoadGlobalCallName now pushes an external function for undefined names.
    • Added Opcode::BuildCell; parser/compiler track captured comprehension slots and VM allocates undefined cells for inlined comprehensions so closures capture targets correctly. Serialization preserves captured cells.
    • REPL/executor store heap and ResourceTracker separately; snapshots expose snapshot.tracker; VM uses heap.tracker()/tracker_mut() where needed.
    • Added indexmap for deterministic capture/name handling; compiler/prepare updated.
  • Migration

    • Implement ContainsHeap with fn heap(&self) -> &Heap and fn dec_ref(&mut self, HeapId).
    • Change params from &Heap/&mut Heap to &HeapReader<'_> where allocating, refcounting, or tracking resources.
    • Replace inline external-function handling with heap-allocated ExtFunction (Value::Ref to HeapData::ExtFunction).
    • Construct heaps with Heap::new(0) and pass a tracker from the VM; update REPL/executor/snapshot code to access tracker directly.

Written for commit faddf6c. Summary will update on new commits.

Review in cubic

@codspeed-hq

codspeed-hq Bot commented Jul 20, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 26 untouched benchmarks
⏩ 16 skipped benchmarks1


Comparing dh/hoist-globals-tracker (f7f9d9b) with main (4aa972b)

Open in CodSpeed

Footnotes

  1. 16 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Results 📊

✅ Patch coverage is 100.00%. Project has 8954 uncovered lines.
✅ Project coverage is 82.34%. Comparing base (base) to head (head).

Coverage diff
@@            Coverage Diff             @@
##          main       #PR       +/-##
==========================================
+ Coverage    82.34%    82.34%        —%
==========================================
  Files          218       218         —
  Lines        50709     50709         —
  Branches    107202    107202         —
==========================================
+ Hits         41755     41755         —
- Misses        8954      8954         —
- Partials      3254      3254         —

Generated by Codecov Action

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 55 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/monty/src/types/tuple.rs">

<violation number="1" location="crates/monty/src/types/tuple.rs:35">
P3: Rustdoc now has an unresolved `Heap::check_time` link in this module, so documentation builds can emit a warning or fail under `-D warnings`. Updating that link to `HeapReader::check_time` keeps the API documentation valid after this import change.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

exception_private::{ExcType, RunResult},
hash::HashValue,
heap::{DropWithContext, Heap, HeapData, HeapId, HeapItem, HeapRead, HeapReadOutput},
heap::{DropWithContext, HeapData, HeapId, HeapItem, HeapRead, HeapReadOutput, HeapReader},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: Rustdoc now has an unresolved Heap::check_time link in this module, so documentation builds can emit a warning or fail under -D warnings. Updating that link to HeapReader::check_time keeps the API documentation valid after this import change.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/src/types/tuple.rs, line 35:

<comment>Rustdoc now has an unresolved `Heap::check_time` link in this module, so documentation builds can emit a warning or fail under `-D warnings`. Updating that link to `HeapReader::check_time` keeps the API documentation valid after this import change.</comment>

<file context>
@@ -32,7 +32,7 @@ use crate::{
     exception_private::{ExcType, RunResult},
     hash::HashValue,
-    heap::{DropWithContext, Heap, HeapData, HeapId, HeapItem, HeapRead, HeapReadOutput},
+    heap::{DropWithContext, HeapData, HeapId, HeapItem, HeapRead, HeapReadOutput, HeapReader},
     intern::StaticStrings,
     resource::{ResourceError, ResourceTracker},
</file context>

@macroscopeapp

macroscopeapp Bot commented Jul 27, 2026

Copy link
Copy Markdown

Approvability

Verdict: Needs human review

Unable to check for correctness in faddf6c. This PR introduces significant runtime behavior changes beyond the title's scope: new comprehension closure capture semantics with a new opcode (BuildCell), restructured external function identity (now heap-based with weak caching), and ResourceTracker architectural changes. These core VM semantic changes warrant careful review.

You can customize Macroscope's approvability policy. Learn more.

@davidhewitt
davidhewitt force-pushed the dh/hoist-globals-tracker branch from f7f9d9b to faddf6c Compare July 27, 2026 11:19
@macroscopeapp

macroscopeapp Bot commented Jul 27, 2026

Copy link
Copy Markdown

Macroscope skipped reviewing this pull request. Per-review cost limit exceeded (workspace setting).

This review would cost an estimated $9.97, which exceeds your per-review limit of $5.00.

The top 3 files driving up this estimate:

File Size Estimate
crates/monty/src/heap.rs 37.89KB $1.89
crates/monty/src/prepare.rs 21.65KB $1.08
crates/monty/src/bytecode/compiler.rs 19.56KB $0.98

Tip

To get this pull request reviewed, you can:

  1. Comment @macroscope-app on this PR to request a manual review (monthly spend limits still apply).
  2. Exclude the file(s) above from review by adding a pattern to your .macroscope/ignore.md — note that creating this file replaces Macroscope's built-in default ignores rather than extending them.
  3. Raise your cost limit in your workspace billing settings.

Turn off this reminder going forward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant