Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The reason we didn't have it here before is that it requires tokio synchronization primitives.
- Removed `EpochGuard`, replaced by `bex_heap::heap_guard::PermitProof`. It depends on the lifetime of an `ActiveHeapPermit` (but without generics) so provides proof (via Rust lifetime guarantees) that an `ActiveHeapPermit` is held. - Removed `with_gc_protection`, just use `ActiveHeapPermit`/`PermitProof`.
Contributor
📝 WalkthroughWalkthroughThis PR replaces closure-based epoch guards with a lifetime-tied Changes
Sequence Diagram(s)sequenceDiagram
participant Runtime as RuntimeIoAdapter
participant PermitMgr as HeapPermitManager
participant SysOp as SysOpFn (generated)
participant Heap as BexHeap
Runtime->>PermitMgr: acquire_permit(call_id)
PermitMgr-->>Runtime: ActiveHeapPermit
Runtime->>Runtime: permit = active.proof()
Runtime->>SysOp: invoke(heap, permit, args, ctx, call_id)
SysOp->>Heap: access fields using (heap, permit)
Heap-->>SysOp: BexValue<'a> / owned conversions
SysOp-->>Runtime: SysOpResult
Runtime->>PermitMgr: drop permit / release(active)
PermitMgr-->>Runtime: permit released
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
baml_language/crates/bex_heap/src/tlab.rs (3)
346-351: Assertion is weaker than the test name implies.
assert_ne!(ptr1, ptr2)only proves the two first allocations aren't literally the same slot; it doesn't prove the two TLABs got non-overlapping chunks (the original test likely checked chunk ranges). Sincealloc_tlab_chunkbumps atomically, this is effectively guaranteed and not a correctness risk, but consider also asserting each chunk'sremaining()is independent (e.g., allocate N < chunk_size fromtlab1only, then checktlab2.remaining()is unchanged) to better match the test's intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bex_heap/src/tlab.rs` around lines 346 - 351, The test currently only asserts ptr1 != ptr2 but should also verify TLAB chunks are independent; after creating tlab1 and tlab2 and before any cross allocations, record tlab2.remaining() (or compute chunk_size), then perform an allocation from tlab1 (e.g., allocate N < chunk_size via tlab1.alloc or call alloc_tlab_chunk on tlab1) and assert tlab2.remaining() is unchanged; reference tlab1, tlab2, alloc, alloc_tlab_chunk, remaining() and chunk_size to locate where to add the extra check so the test proves non-overlapping chunk allocation.
327-335: Weak post-condition for compile-time isolation.
is_compile_time_ptr(ptr)confirms the runtime allocation isn't in the compile-time region, which is the main thing the old assertion was trying to express, so this is fine. Optional: strengthen by also reading back the value (e.g.,ptr.get()→Object::String("runtime")) to prove the write actually landed in runtime space rather than only checking the pointer classification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bex_heap/src/tlab.rs` around lines 327 - 335, The assertion only checks ptr classification; strengthen the post-condition by reading back the stored object and asserting its value matches the runtime string. After calling Tlab::alloc(Object::String("runtime".to_string())) (with Tlab::new and tlab.alloc), fetch the object at ptr (e.g., via heap.get(ptr) or ptr.get()) and add an assert that the fetched value equals Object::String("runtime"), while keeping the existing assert using heap.is_compile_time_ptr(ptr).
663-673: Align the dedup key styles for consistency.Both tests compile and work correctly—
HeapPtrderivesCopyand implementsHash/Equsing only the raw pointer field (self.ptr), soseen.insert(ptr.as_ptr() as usize)(line 668) andseen.insert(*ptr)(line 732) detect duplicates identically. Aligning them on one style improves readability and makes the "uniqueness = pointer identity" invariant explicit.Suggested alignment to match test_miri_concurrent_tlab_allocation:
- assert!( - seen.insert(*ptr), - "Duplicate pointer from concurrent refill" - ); + assert!( + seen.insert(ptr.as_ptr() as usize), + "Duplicate pointer {:?} from concurrent refill", + ptr.as_ptr() + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bex_heap/src/tlab.rs` around lines 663 - 673, The deduplication uses two different key styles; align them to the HeapPtr raw-key style used elsewhere: in the loop over all_pointers replace seen.insert(ptr.as_ptr() as usize) with seen.insert(*ptr) (or vice versa consistently) so the HashSet key matches HeapPtr's Hash/Eq implementation; update the assert message to still include ptr.as_ptr() for human-readable pointer output while using *ptr as the dedup key (symbols: seen, all_pointers, ptr, HeapPtr).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 715-729: The resolve_handle function currently returns a raw
HeapPtr while ignoring the PermitProof, which allows callers to drop the permit
and use the pointer after GC; change resolve_handle to accept a closure (e.g.,
fn resolve_handle<F, R>(&self, permit: bex_heap::PermitProof<'_>, handle:
&bex_external_types::Handle, f: F) -> Option<R> where F: for<'a>
FnOnce(HeapPtrBoundTo<'a>) -> R) and call self.heap.resolve_handle_ptr inside
that closure so the resolved pointer is only available within the permit-tied
callback (mirror the pattern used in accessor.rs/resolve_handle_ptr usage),
update call sites to pass closures, and add a unit test that attempts to cache
the pointer across a dropped permit to assert it is impossible/unsafe or that
the API prevents it.
In `@baml_language/crates/bex_heap/src/accessor.rs`:
- Around line 173-179: Change the permit parameter lifetime to tie it to the
returned borrows: update the signatures of as_object, as_string, as_array,
as_map, as_class, as_enum, and as_builtin_class to take PermitProof<'a> instead
of PermitProof<'_> (and adjust any internal helper types/closures that accept
the permit) so the proof lifetime 'a is the same as the returned &'a String /
Vec<BexValue<'a>> / BexClass<'a> etc.; also update the internal as_object helper
signature and any places that call it to propagate 'a, and modify the codegen
templates baml_builtins_macros/src/codegen_accessors.rs and
baml_builtins2_codegen/src/codegen_io.rs so generated wrappers use
PermitProof<'a> accordingly.
---
Nitpick comments:
In `@baml_language/crates/bex_heap/src/tlab.rs`:
- Around line 346-351: The test currently only asserts ptr1 != ptr2 but should
also verify TLAB chunks are independent; after creating tlab1 and tlab2 and
before any cross allocations, record tlab2.remaining() (or compute chunk_size),
then perform an allocation from tlab1 (e.g., allocate N < chunk_size via
tlab1.alloc or call alloc_tlab_chunk on tlab1) and assert tlab2.remaining() is
unchanged; reference tlab1, tlab2, alloc, alloc_tlab_chunk, remaining() and
chunk_size to locate where to add the extra check so the test proves
non-overlapping chunk allocation.
- Around line 327-335: The assertion only checks ptr classification; strengthen
the post-condition by reading back the stored object and asserting its value
matches the runtime string. After calling
Tlab::alloc(Object::String("runtime".to_string())) (with Tlab::new and
tlab.alloc), fetch the object at ptr (e.g., via heap.get(ptr) or ptr.get()) and
add an assert that the fetched value equals Object::String("runtime"), while
keeping the existing assert using heap.is_compile_time_ptr(ptr).
- Around line 663-673: The deduplication uses two different key styles; align
them to the HeapPtr raw-key style used elsewhere: in the loop over all_pointers
replace seen.insert(ptr.as_ptr() as usize) with seen.insert(*ptr) (or vice versa
consistently) so the HashSet key matches HeapPtr's Hash/Eq implementation;
update the assert message to still include ptr.as_ptr() for human-readable
pointer output while using *ptr as the dedup key (symbols: seen, all_pointers,
ptr, HeapPtr).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f0bbc2d0-9182-4590-9df9-588f87c4875b
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
baml_language/crates/baml_builtins2_codegen/src/codegen_io.rsbaml_language/crates/baml_builtins_codegen/src/codegen_accessors.rsbaml_language/crates/baml_builtins_macros/src/codegen_accessors.rsbaml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/heap_guard.rsbaml_language/crates/bex_external_types/src/epoch_guard.rsbaml_language/crates/bex_external_types/src/handle.rsbaml_language/crates/bex_external_types/src/lib.rsbaml_language/crates/bex_heap/Cargo.tomlbaml_language/crates/bex_heap/src/accessor.rsbaml_language/crates/bex_heap/src/heap_guard.rsbaml_language/crates/bex_heap/src/lib.rsbaml_language/crates/bex_heap/src/tlab.rsbaml_language/crates/bex_project/src/bex.rsbaml_language/crates/bex_vm_types/src/roots.rsbaml_language/crates/sys_ops/src/lib.rsbaml_language/crates/sys_types/src/lib.rs
💤 Files with no reviewable changes (3)
- baml_language/crates/bex_external_types/src/lib.rs
- baml_language/crates/bex_external_types/src/epoch_guard.rs
- baml_language/crates/bex_external_types/src/handle.rs
Merging this PR will not alter performance
|
Binary size checks passed✅ 7 passed
Generated by |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
baml_language/crates/bex_heap/src/tlab.rs (2)
727-736: Nit: unify the duplicate-detection key across the two concurrent TLAB tests.
test_miri_concurrent_tlab_allocation(lines 664–672) insertsptr.as_ptr() as usizeinto theHashSet, whereas this test inserts*ptr(relying onHeapPtr: Copy + Hash + Eq). Both are correct ifHeapPtr'sHash/Eqare based on the underlying pointer address, but the divergent styles make it easy to assume the two tests check different invariants.Pick one form for consistency — either hash
HeapPtrdirectly in both, or go throughas_ptr() as usizein both.♻️ Suggested consolidation (mirror the other test)
- let mut seen = std::collections::HashSet::new(); - for thread_pointers in &all_pointers { - for ptr in thread_pointers { - assert!( - seen.insert(*ptr), - "Duplicate pointer from concurrent refill" - ); - } - } + let mut seen = std::collections::HashSet::new(); + for thread_pointers in &all_pointers { + for ptr in thread_pointers { + assert!( + seen.insert(ptr.as_ptr() as usize), + "Duplicate pointer {:?} from concurrent refill", + ptr.as_ptr() + ); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bex_heap/src/tlab.rs` around lines 727 - 736, The two concurrent TLAB tests use different keys for duplicate detection—one inserts ptr.as_ptr() as usize, the other inserts *ptr—so unify them for consistency; update the duplicate-check in tlab.rs to use the same form as test_miri_concurrent_tlab_allocation (convert each HeapPtr to a raw address via as_ptr() as usize before inserting into the HashSet), adjust the HashSet type to HashSet<usize> if needed, and ensure the assert message remains the same so both tests verify identical invariants.
346-351: Nit: assertion is weaker than the test name suggests.The test is named
test_multiple_tlabs_no_overlap, but only a single object is allocated from each TLAB and the assertion is justassert_ne!(ptr1, ptr2). That catches the worst case (identical chunk), but doesn't verify the TLAB regions are disjoint. Consider either renaming to reflect the current scope (e.g.test_multiple_tlabs_distinct_first_pointer) or strengthening by allocating enough from each TLAB to demonstrate non-overlapping ranges viaheap.is_compile_time_ptr/ pointer comparisons. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/bex_heap/src/tlab.rs` around lines 346 - 351, The test test_multiple_tlabs_no_overlap only allocates one object per TLAB (using tlab1.alloc and tlab2.alloc) and then asserts ptr1 != ptr2, which doesn't prove regions are disjoint; either rename the test to reflect the weaker check or strengthen it by allocating multiple objects from each TLAB (call tlab1.alloc and tlab2.alloc in a loop) and then assert the address ranges do not overlap using pointer comparisons or heap.is_compile_time_ptr/other range-check helpers (compare lowest and highest pointers from each TLAB or verify one TLAB's high pointer is less than the other's low pointer) to demonstrate non-overlapping TLAB regions.baml_language/crates/baml_builtins_codegen/src/codegen_accessors.rs (1)
435-444: UsePermitProof<'a>ininto_ownedfor consistency and robustness.The generated accessor methods require
PermitProof<'a>(line 397), andinto_ownedforwards thepermitparameter into them (e.g.,self.#name(heap, permit)?at lines 350, 354, 356, 362). UsingPermitProof<'_>ininto_ownedintroduces a fresh, unconstrained lifetime that relies on variance and inference to satisfy the accessor's expectedPermitProof<'a>— fragile and inconsistent withbaml_builtins2_codegen/src/codegen_io.rs, which usesPermitProof<'a>for its equivalent method.Proposed change
pub fn into_owned( self, heap: &'a BexHeap, - permit: PermitProof<'_>, + permit: PermitProof<'a>, ) -> Result<owned::`#owned_name`, AccessError> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_builtins_codegen/src/codegen_accessors.rs` around lines 435 - 444, The into_owned method currently takes permit: PermitProof<'_> which creates a fresh unconstrained lifetime; change its signature to accept permit: PermitProof<'a> so it matches the accessor methods (e.g., self.#name(heap, permit)?), the generated accessor lifetimes, and the pattern used in baml_builtins2_codegen/src/codegen_io.rs; update the into_owned declaration in the impl (the function on type owning owned::`#owned_name` that also takes heap: &'a BexHeap) to use PermitProof<'a> to ensure lifetime consistency and robustness when forwarding permit to the accessors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@baml_language/crates/baml_builtins_codegen/src/codegen_accessors.rs`:
- Around line 435-444: The into_owned method currently takes permit:
PermitProof<'_> which creates a fresh unconstrained lifetime; change its
signature to accept permit: PermitProof<'a> so it matches the accessor methods
(e.g., self.#name(heap, permit)?), the generated accessor lifetimes, and the
pattern used in baml_builtins2_codegen/src/codegen_io.rs; update the into_owned
declaration in the impl (the function on type owning owned::`#owned_name` that
also takes heap: &'a BexHeap) to use PermitProof<'a> to ensure lifetime
consistency and robustness when forwarding permit to the accessors.
In `@baml_language/crates/bex_heap/src/tlab.rs`:
- Around line 727-736: The two concurrent TLAB tests use different keys for
duplicate detection—one inserts ptr.as_ptr() as usize, the other inserts *ptr—so
unify them for consistency; update the duplicate-check in tlab.rs to use the
same form as test_miri_concurrent_tlab_allocation (convert each HeapPtr to a raw
address via as_ptr() as usize before inserting into the HashSet), adjust the
HashSet type to HashSet<usize> if needed, and ensure the assert message remains
the same so both tests verify identical invariants.
- Around line 346-351: The test test_multiple_tlabs_no_overlap only allocates
one object per TLAB (using tlab1.alloc and tlab2.alloc) and then asserts ptr1 !=
ptr2, which doesn't prove regions are disjoint; either rename the test to
reflect the weaker check or strengthen it by allocating multiple objects from
each TLAB (call tlab1.alloc and tlab2.alloc in a loop) and then assert the
address ranges do not overlap using pointer comparisons or
heap.is_compile_time_ptr/other range-check helpers (compare lowest and highest
pointers from each TLAB or verify one TLAB's high pointer is less than the
other's low pointer) to demonstrate non-overlapping TLAB regions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 081b237e-1609-4544-be37-78582c0f383f
📒 Files selected for processing (6)
baml_language/crates/baml_builtins2_codegen/src/codegen_io.rsbaml_language/crates/baml_builtins_codegen/src/codegen_accessors.rsbaml_language/crates/baml_builtins_macros/src/codegen_accessors.rsbaml_language/crates/bex_heap/src/accessor.rsbaml_language/crates/bex_heap/src/tlab.rsbaml_language/crates/sys_types/src/lib.rs
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.
bex_heap. This just requires thatbex_heaphave tokio as a dependency for synchronization primitives. This is fine because it is always used alongsidebex_engine, which already has tokio.EpochGuardwithPermitProofwhich integrates with the heap guard systemwith_gc_protectionto also just use the heap guard system.Summary by CodeRabbit
Release Notes
Refactor
Tests