Vibe GC #2386
Vibe GC #2386
Conversation
cg-jl
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
cg-jl
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
cg-jl
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
🌿 Preview your docs: https://boundary-preview-9c2f2716-c19f-4179-b745-20686211173f.docs.buildwithfern.com |
|
🔒 Entelligence AI Vulnerability Scanner ✅ No security vulnerabilities found! Your code passed our comprehensive security analysis. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 65b5fe7 in 2 minutes and 52 seconds. Click for details.
- Reviewed
1036lines of code in6files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-vm/src/native.rs:23
- Draft comment:
Avoid using 'Object::Null' as a fallback in error messages – the Object enum doesn’t define a Null variant. Use a proper error value instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Without seeing the Object enum definition, I can't be 100% sure Object::Null doesn't exist. The suggestion to panic seems worse than using a null fallback for error messages. The original code change appears to be making error handling more robust, not less. I don't have visibility into the Object enum definition to verify if Null exists. The comment could be correct about the technical detail. Even if Object::Null doesn't exist, suggesting a panic is worse than using a null placeholder for error messages. The code change is making error handling more robust overall. Delete the comment. The suggestion would make the code worse by introducing panics, and we lack context to verify the claim about Object::Null.
2. engine/baml-vm/src/vm.rs:762
- Draft comment:
In fulfil_future, only the future at the top of the stack is updated. If the same future appears elsewhere, it won’t be replaced. Confirm that this is the intended behavior. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. engine/baml-vm/src/vm.rs:1388
- Draft comment:
Document the use of 'skip(1)' after draining the stack in DispatchFuture. It’s not immediately clear why the first element is discarded – clarifying the intended stack layout would help. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. engine/baml-vm/src/vm.rs:1590
- Draft comment:
The slicing logic for keys and values in AllocMap is quite intricate. Consider adding more inline documentation or refactoring for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. engine/baml-vm/src/vm.rs:796
- Draft comment:
Multiple calls to 'self.objects.get(...)' on the same index are repeated in error branches. Consider caching the result in a local variable to improve clarity and avoid potential duplicate lookups. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. engine/baml-vm/src/vm.rs:1521
- Draft comment:
Typographical error: "totaly" should be "totally". - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_BtO1h58sRPqHVvj9
You can customize 
engine/baml-vm/src/vm.rs
Outdated
| return Err(InternalError::TypeError { | ||
| expected: ObjectType::Instance.into(), | ||
| got: ObjectType::of(&self.objects[reference]).into(), | ||
| got: ObjectType::of(self.objects.get(reference).unwrap_or(&Object::Null)).into(), |
There was a problem hiding this comment.
Avoid using 'Object::Null' as a fallback in error reporting – the Object enum doesn’t have a Null variant, which makes these unwraps potentially misleading.
| got: ObjectType::of(self.objects.get(reference).unwrap_or(&Object::Null)).into(), | |
| got: ObjectType::of(self.objects.get(reference).unwrap_or(&Object::Stale)).into(), |
Review Summary🏷️ Draft Comments (4)
|
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
engine/baml-vm/src/vm.rs
Outdated
| /// Stale variant for freed/uninitialized slots in the object pool. | ||
| Stale, | ||
|
|
There was a problem hiding this comment.
correctness: The new Object::Stale variant is not handled in ObjectPool::get()/get_mut() or in VM logic, so accessing a stale/freed object may return Object::Stale and cause silent logic errors instead of raising StaleObjectReference.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In engine/baml-vm/src/vm.rs, ensure that all accesses to objects via `ObjectPool::get()`/`get_mut()` and all VM logic that matches on `Object` properly detect and error on `Object::Stale`, returning `InternalError::StaleObjectReference` instead of allowing silent logic errors. Update all relevant match arms and accessors to enforce this contract.
engine/baml-vm/src/vm.rs
Outdated
| pub(crate) fn as_object( | ||
| &self, | ||
| value: &Value, | ||
| object_type: ObjectType, | ||
| ) -> Result<ObjectIndex, InternalError> { | ||
| let Value::Object(index) = value else { | ||
| return Err(InternalError::TypeError { | ||
| expected: object_type.into(), | ||
| got: self.type_of(value), | ||
| }); | ||
| }; | ||
|
|
||
| Ok(*index) | ||
| } |
There was a problem hiding this comment.
correctness: The as_object helper in Vm does not check for stale object slots, so it may return a valid index for a stale/freed object, leading to undefined behavior.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In engine/baml-vm/src/vm.rs, lines 796-809, update the `as_object` method to check if the object at the given index is `Object::Stale`. If so, return `InternalError::StaleObjectReference`. This prevents returning indices for freed objects and ensures stale references are never treated as valid.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| pub(crate) fn as_object( | |
| &self, | |
| value: &Value, | |
| object_type: ObjectType, | |
| ) -> Result<ObjectIndex, InternalError> { | |
| let Value::Object(index) = value else { | |
| return Err(InternalError::TypeError { | |
| expected: object_type.into(), | |
| got: self.type_of(value), | |
| }); | |
| }; | |
| Ok(*index) | |
| } | |
| pub(crate) fn as_object( | |
| &self, | |
| value: &Value, | |
| object_type: ObjectType, | |
| ) -> Result<ObjectIndex, InternalError> { | |
| let Value::Object(index) = value else { | |
| return Err(InternalError::TypeError { | |
| expected: object_type.into(), | |
| got: self.type_of(value), | |
| }); | |
| }; | |
| match self.objects.get(*index) { | |
| Ok(Object::Stale) => Err(InternalError::StaleObjectReference), | |
| Ok(obj) => Ok(*index), | |
| Err(e) => Err(e), | |
| } | |
| } |
engine/baml-vm/src/vm.rs
Outdated
| pub fn collect_garbage(&mut self) { | ||
| self.objects.drain(self.runtime_allocs_offset..); | ||
| // For now, just a placeholder until we implement the full GC | ||
| // This will be replaced with the mark-sweep implementation | ||
| self.objects.truncate(self.runtime_allocs_offset); | ||
| self.mark_bits.truncate(self.runtime_allocs_offset); | ||
| self.free_list.clear(); | ||
| } |
There was a problem hiding this comment.
performance: collect_garbage currently truncates all runtime objects instead of performing a mark-and-sweep, causing O(n) object leaks and unbounded memory growth in long-running VMs.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
Implement a real mark-and-sweep generational garbage collector in engine/baml-vm/src/vm.rs, lines 775-781. The current `collect_garbage` just truncates all runtime objects, which causes O(n) memory leaks and prevents long-running VMs from reclaiming unused memory. Replace this with a proper mark phase (traverse all roots, mark reachable objects using `mark_bits`), then a sweep phase (free unreachable objects, update `free_list`, and reset mark bits). Ensure all object references (stack, globals, escaped_futures, etc.) are considered roots.
engine/baml-vm/src/vm.rs
Outdated
| let index = self.objects.len(); | ||
| self.objects.push(ObjectSlot { | ||
| object: Object::Array(values), | ||
| generation: 0, | ||
| }); | ||
| self.mark_bits.push(false); | ||
| Value::Object(ObjectIndex::from_raw(index)) | ||
| } |
There was a problem hiding this comment.
performance: Object allocation (e.g., in alloc_array, AllocArray, AllocInstance, DispatchFuture, AllocMap) always appends to the pool, never reusing freed slots, causing unbounded memory growth and poor cache locality.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
Refactor all object allocation sites in engine/baml-vm/src/vm.rs (lines 785-792, 1122-1131, 1335-1347, 1397-1406, 1616-1624) to reuse freed slots from `free_list` before appending to the object pool. When allocating, check `free_list` for available indices, update the slot and its generation, and only append if no free slots exist. This prevents unbounded memory growth and improves memory locality.
This reverts commit 65b5fe7.
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
|
🌿 Preview your docs: https://boundary-preview-f8eb9430-7b2c-434c-b18e-afc48cc9ce64.docs.buildwithfern.com |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Important
Introduces garbage collection improvements for the
Vibecomponent, optimizing memory management and handling edge cases.Vibecomponent, optimizing memory management.This description was created by
for a6bdf0d. You can customize this summary. It will automatically update as commits are pushed.