BoehmGC initial support #232
Conversation
WalkthroughThe updates encompass enhancements and modifications across various components of the runtime and VM, focusing on memory allocation and garbage collection. Key changes include the introduction of new classes and interfaces for garbage collection layouts, updates to method signatures to support different kinds of allocations, and the addition of a new conditional compilation flag for experimental features. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (17)
- runtime/common/reflection/VeinClass.cs (1 hunks)
- runtime/common/reflection/VeinModule.cs (1 hunks)
- runtime/ishtar.vm/ishtar.vm.csproj (1 hunks)
- runtime/ishtar.vm/runtime/IshtarGC.cs (11 hunks)
- runtime/ishtar.vm/runtime/allocators/DebugManagedAllocator.cs (2 hunks)
- runtime/ishtar.vm/runtime/allocators/GCLayoutAllocator.cs (1 hunks)
- runtime/ishtar.vm/runtime/allocators/IIshtarAllocator.cs (1 hunks)
- runtime/ishtar.vm/runtime/allocators/IIshtarAllocatorPool.cs (1 hunks)
- runtime/ishtar.vm/runtime/allocators/IshtarAllocatorPool.cs (3 hunks)
- runtime/ishtar.vm/runtime/allocators/NativeMemory_WindowsAllocator.cs (1 hunks)
- runtime/ishtar.vm/runtime/allocators/NullAllocator.cs (1 hunks)
- runtime/ishtar.vm/runtime/allocators/WindowsHeapAllocator.cs (1 hunks)
- runtime/ishtar.vm/runtime/vm/CallFrame.cs (2 hunks)
- runtime/ishtar.vm/runtime/vm/RuntimeIshtarClass.cs (2 hunks)
- runtime/ishtar.vm/vm.switch.cs (1 hunks)
- test/ishtar_test/IshtarTestBase.cs (1 hunks)
- vein_lang.sln.DotSettings (1 hunks)
Files skipped from review due to trivial changes (1)
- vein_lang.sln.DotSettings
Additional comments not posted (31)
runtime/ishtar.vm/runtime/allocators/IIshtarAllocatorPool.cs (1)
6-6: The addition ofAllocationKindin theRentmethod signature aligns well with the broader updates to the allocator interfaces.runtime/ishtar.vm/runtime/allocators/IIshtarAllocator.cs (1)
14-19: The updated method signatures forAllocZeroedwith theAllocationKindparameter are consistent and necessary for the new memory management enhancements.runtime/ishtar.vm/runtime/allocators/NullAllocator.cs (1)
8-16: The updatedAllocZeroedmethods inNullAllocatorcorrectly throwNotImplementedException, aligning with its role as a non-functional allocator for certain scenarios.runtime/ishtar.vm/runtime/allocators/NativeMemory_WindowsAllocator.cs (1)
21-33: The updatedAllocZeroedmethods inNativeMemory_WindowsAllocatorcorrectly delegate to theAllocatemethod, ensuring proper memory management and alignment with the new memory allocation strategies.runtime/ishtar.vm/vm.switch.cs (1)
44-44: The addition of theDisabledFinalizationproperty inVMConfigis well-implemented, providing a dynamic way to manage finalization features based on configuration.runtime/ishtar.vm/runtime/allocators/GCLayoutAllocator.cs (1)
5-37: TheGCLayoutAllocatorclass is well-implemented, correctly managing memory allocations based on the providedGCLayoutand aligning with the new garbage collection mechanisms.runtime/ishtar.vm/runtime/allocators/IshtarAllocatorPool.cs (1)
Line range hint
5-52: The updates toIshtarAllocatorPool, including the integration ofGCLayoutin the constructor and the modification of theRentmethod to includeAllocationKind, are well-implemented and crucial for the new garbage collection mechanisms.runtime/ishtar.vm/ishtar.vm.csproj (1)
23-23: The inclusion of theBOEHM_GCdefinition in theDefineConstantsof the debug configuration is correctly implemented, aligning with the PR's objectives to integrate Boehm's garbage collector.runtime/ishtar.vm/runtime/allocators/DebugManagedAllocator.cs (5)
33-33: LGTM! The method correctly handles the newAllocationKindparameter.
39-39: LGTM! Consistent handling ofAllocationKindacross different method overloads.
45-45: LGTM! Proper handling ofUIntPtrsize parameter with consistent logic.
51-51: LGTM! Consistent handling ofIntPtrsize parameter.
57-57: LGTM! Correct handling ofintsize parameter, maintaining consistency with other overloads.runtime/ishtar.vm/runtime/allocators/WindowsHeapAllocator.cs (5)
43-43: LGTM! The method correctly handles the newAllocationKindparameter by delegating toAllocFromHeap.
46-46: LGTM! Consistent handling ofAllocationKindacross different method overloads.
49-49: LGTM! Proper handling ofUIntPtrsize parameter with consistent logic.
52-52: LGTM! Consistent handling ofIntPtrsize parameter.
55-55: LGTM! Correct handling ofintsize parameter, maintaining consistency with other overloads.runtime/common/reflection/VeinModule.cs (1)
131-131: LGTM! The introduction ofGC_FINALIZER_BARRIERaligns with the enhancements to the garbage collection mechanisms.test/ishtar_test/IshtarTestBase.cs (1)
16-16: LGTM! The inclusion ofishtar.vm.runtimenamespace is necessary for consistency in the testing environment.runtime/common/reflection/VeinClass.cs (1)
73-73: LGTM! The refinement to include constructors and deconstructors in theGetOrCreateTormethod enhances its specificity.runtime/ishtar.vm/runtime/vm/RuntimeIshtarClass.cs (2)
310-310: LGTM! The override ofGetDefaultDtorto returnRuntimeIshtarMethodaligns with the custom behaviors for runtime classes.
311-311: LGTM! The override ofGetDefaultCtorto returnRuntimeIshtarMethodensures consistency and correctness in runtime class behavior.runtime/ishtar.vm/runtime/IshtarGC.cs (8)
14-14: Introduced a new delegateIshtarFinalizationProcfor finalization procedures.
18-39: IntroducedGCLayoutinterface with a comprehensive set of methods for garbage collection operations.
41-45: IntroducedGCLayout_Debuginterface for debugging garbage collection.
263-266: Conditional compilation for Boehm GC. Ensure that theBOEHM_GCflag is properly managed across different build configurations.
365-365: Instantiation ofIshtarAllocatorPoolwithgcLayout. Ensure thatgcLayoutis correctly initialized before this point to avoid null reference issues.
459-459: Usage ofallocatorPool.RentwithAllocationKind.no_reference. Verify that this allocation kind is handled correctly in all scenarios.
Line range hint
720-750: Significant changes inAllocObjectmethod to support Boehm GC. Ensure that all new logic, especially around finalizers and object registration, is correct and does not introduce memory leaks or other issues.
810-833: Introduced methods for object lifecycle management with Boehm GC. Ensure that these methods are correctly used and do not lead to improper memory management.Verification successful
The provided script outputs show the contexts in which the methods
IsAlive,ObjectRegisterFinalizer, andCollectare used within theIshtarGC.csfile. Here's a breakdown of the findings:
IsAlive: This method is used within a finalizer (
_direct_finalizer) to check if an object is still valid before proceeding with finalization logic. This is a correct and expected use of the method, ensuring that operations are only performed on valid objects.ObjectRegisterFinalizer: This method is used in two contexts:
- Directly after object allocation (
AllocObject), a finalizer is registered. This is a standard practice to ensure that resources are cleaned up when the object is no longer needed.- Within the
_direct_finalizeritself, potentially to re-register a finalizer under certain conditions. This usage is less typical and could be prone to errors if not handled carefully, especially in terms of re-entrancy or double finalization issues. However, without specific details on the logic inside_direct_finalizer, it's hard to definitively say if this is incorrect.Collect: The
Collectmethod is invoked in various cleanup and error handling scenarios to perform garbage collection. This is a standard use of such a method in a garbage-collected environment.From the analysis, it appears that the methods are generally used correctly. The potential concern with
ObjectRegisterFinalizerbeing used within a finalizer itself warrants a closer look to ensure it doesn't lead to unintended behaviors like double finalization. However, this would typically require a deeper understanding of the application logic and possibly runtime debugging or logging to fully validate.Conclusion
The methods related to object lifecycle management in the Boehm GC implementation are used in contexts that align with typical garbage collection patterns. There is a potential area of concern with the re-registration of finalizers within a finalizer, which could be further investigated or documented to ensure it doesn't lead to issues.
- Ensure that the re-registration of finalizers within
_direct_finalizeris intentional and safe.- Consider adding more detailed logging or error handling around finalizer re-registration to catch any potential issues in runtime.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct usage of object lifecycle management methods with Boehm GC. # Test: Search for usages of lifecycle management methods (`IsAlive`, `ObjectRegisterFinalizer`, etc.) and their correctness. rg --type cs "IsAlive|ObjectRegisterFinalizer|Collect"Length of output: 15594
Script:
#!/bin/bash # Extracting the context of usage for the methods `IsAlive`, `ObjectRegisterFinalizer`, and `Collect` to ensure they are used correctly. # Context for `IsAlive` rg --type cs --context 5 "IsAlive" runtime/ishtar.vm/runtime/IshtarGC.cs # Context for `ObjectRegisterFinalizer` rg --type cs --context 5 "ObjectRegisterFinalizer" runtime/ishtar.vm/runtime/IshtarGC.cs # Context for `Collect` rg --type cs --context 5 "Collect" runtime/ishtar.vm/runtime/IshtarGC.csLength of output: 4350
| public static extern void GC_finalize_all(); | ||
|
|
||
| [UnmanagedFunctionPointer(CallingConvention.Cdecl)] | ||
| public delegate void GC_stop_func(); | ||
|
|
||
|
|
||
| // GC_API void GC_CALL GC_set_find_leak(int); | ||
| [DllImport(LIBNAME)] | ||
| public static extern void GC_set_find_leak(bool zero); | ||
|
|
||
| [DllImport(LIBNAME)] | ||
| public static extern long get_heap_size(); | ||
|
|
||
| [DllImport(LIBNAME)] | ||
| public static extern long get_free_bytes(); | ||
|
|
||
| [DllImport(LIBNAME)] | ||
| public static extern long GC_get_heap_usage(); | ||
|
|
||
| [DllImport(LIBNAME)] | ||
| public static extern bool GC_is_marked(void* ptr); | ||
| } | ||
|
|
||
|
|
||
| public void init() | ||
| { | ||
| Native.GC_init(); | ||
| Native.GC_set_find_leak(true); | ||
| } | ||
|
|
||
| public void destroy() => Native.GC_deinit(); | ||
|
|
||
| public void* alloc(uint size) => Native.GC_debug_malloc(size, "empty.vein", 0); | ||
|
|
||
| /* Explicitly deallocate an object. Dangerous if used incorrectly. */ | ||
| /* Requires a pointer to the base of an object. */ | ||
| /* An object should not be enabled for finalization (and it should not */ | ||
| /* contain registered disappearing links of any kind) when it is */ | ||
| /* explicitly deallocated. */ | ||
| /* GC_free(0) is a no-op, as required by ANSI C for free. */ | ||
| public void free(void* ptr) => Native.GC_debug_free(ptr, "empty.vein", 0); | ||
|
|
||
| private static nuint hide_pointer(void* p) => ~(nuint)p; | ||
|
|
||
| public void link(void** link_addr, void* obj, bool trackable) | ||
| { | ||
| *link_addr = (void*)hide_pointer(obj); | ||
| if (trackable) | ||
| Native.GC_register_long_link(link_addr, obj); | ||
| else | ||
| Native.GC_general_register_disappearing_link(link_addr, obj); | ||
| } | ||
|
|
||
| public void unlink(void** link_addr, bool trackable) | ||
| { | ||
| if (trackable) | ||
| Native.GC_unregister_long_link(link_addr); | ||
| else | ||
| Native.GC_unregister_disappearing_link(link_addr); | ||
| *link_addr = null; | ||
| } | ||
|
|
||
| public void link(void** child, void* parent) => Native.GC_general_register_disappearing_link(child, parent); | ||
| public void register_finalizer_ignore_self(void* obj, IshtarFinalizationProc proc) => Native.GC_register_finalizer_ignore_self(obj, proc, null, null, null); | ||
|
|
||
| public void* alloc_atomic(uint size) => throw new NotImplementedException(); | ||
|
|
||
| public void* alloc_immortal(uint size) => throw new NotImplementedException(); | ||
|
|
||
| public void add_roots(void* ptr) => throw new NotImplementedException(); | ||
|
|
||
| public long get_free_bytes() => Native.get_free_bytes(); | ||
|
|
||
| public long get_heap_size() => Native.get_heap_size(); | ||
|
|
||
| public long get_heap_usage() => Native.GC_get_heap_usage(); | ||
|
|
||
| public bool try_collect() => Native.GC_try_to_collect(0) == 1; | ||
|
|
||
| public void collect() => Native.GC_gcollect(); | ||
| public bool is_marked(void* obj) => Native.GC_is_marked(obj); | ||
|
|
||
| public void finalize_all() => Native.GC_finalize_all(); | ||
|
|
||
| public void finalize_on_demand() => throw new NotImplementedException(); | ||
|
|
||
| public void find_leak() => throw new NotImplementedException(); | ||
|
|
||
|
|
||
| public void register_finalizer_no_order(IshtarObject* obj, IshtarFinalizationProc proc, CallFrame frame) | ||
| => Native.GC_debug_register_finalizer_no_order(obj, proc, null, null, null, frame.Debug_GetFile(), frame.Debug_GetLine()); | ||
|
|
||
|
|
||
| public void dump(string file) | ||
| { | ||
| //var ptr = Marshal.StringToHGlobalAnsi(file); | ||
| //Native.GC_dump((void*)ptr); | ||
| //Marshal.ZeroFreeGlobalAllocAnsi(ptr); | ||
| } | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The search for unit or integration tests specifically targeting the BoehmGCLayout class did not yield any results. This absence of test cases in the search results suggests that there might not be adequate testing implemented for the BoehmGCLayout class, particularly concerning its integration with the Boehm garbage collector and aspects like memory management and thread safety.
Given the critical nature of garbage collection in managing application memory and ensuring performance and stability, the lack of tests is a significant concern. It is advisable to implement comprehensive tests that cover various scenarios including stress tests, concurrency tests, and memory leak tests to ensure the robustness of the BoehmGCLayout integration.
Action Required:
- Implement unit and integration tests for the
BoehmGCLayoutclass to verify its functionality, memory management, and thread safety. - Ensure that these tests cover a wide range of scenarios to thoroughly evaluate the garbage collector's performance under different conditions.
Analysis chain
Implemented BoehmGCLayout class which integrates native Boehm GC functions. Ensure thorough testing, especially for memory management and thread safety.
The search results indicate that the BoehmGCLayout class is indeed being used within the IshtarGC.cs file, specifically it is instantiated and assigned to a private field gcLayout. However, the results do not provide information about any specific tests or further usage details that would confirm thorough testing or integration quality, particularly concerning memory management and thread safety as mentioned in the review comment.
To verify the thoroughness of testing and the integration quality, we need to look for unit tests or integration tests that specifically target the BoehmGCLayout class. This will help us understand if the memory management and thread safety aspects are being adequately tested.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration and functionality of BoehmGCLayout with Boehm GC.
# Test: Search for BoehmGCLayout usage and related tests.
rg --type cs "BoehmGCLayout"
Length of output: 226
Script:
#!/bin/bash
# Description: Search for unit or integration tests specifically targeting the BoehmGCLayout class.
rg --type cs -A 30 "class.*Test" | grep "BoehmGCLayout"
Length of output: 55
| public string Debug_GetFile() | ||
| { | ||
| // TODO fetch from dbg symbols | ||
|
|
||
| var str = new StringBuilder(); | ||
|
|
||
| if (this.method.Owner is not null) | ||
| str.AppendLine($"\tat {this.method.Owner.FullName.NameWithNS}.{this.method.Name}"); | ||
| else | ||
| str.AppendLine($"\tat <sys>.{this.method.Name}"); | ||
|
|
||
| var r = this.parent; | ||
|
|
||
| while (r != null) | ||
| { | ||
| str.AppendLine($"\tat {r.method.Owner.FullName.NameWithNS}.{r.method.Name}"); | ||
| r = r.parent; | ||
| } | ||
|
|
||
| return str.ToString(); | ||
| } |
There was a problem hiding this comment.
LGTM! The method constructs a call stack string correctly. Remember to address the TODO for fetching file information from debug symbols.
Would you like me to help implement the debug symbol fetching or should I open a GitHub issue to track this task?
| return str.ToString(); | ||
| } | ||
|
|
||
| public int Debug_GetLine() => 0;// TODO fetch from dbg symbols |
There was a problem hiding this comment.
Placeholder implementation for line number fetching. Ensure to address the TODO for integrating debug symbol fetching.
Would you like me to help implement the debug symbol fetching or should I open a GitHub issue to track this task?
|
|
||
| public unsafe class BoehmGCLayout : GCLayout, GCLayout_Debug | ||
| { | ||
| public class Native |
There was a problem hiding this comment.
не совсем понял зачем этот класс нужен. либа не умеет сама платформу детектить? или ты планировал расширять?
There was a problem hiding this comment.
Ну просто обычно экстерны пихают во внутренний класс обертки
а препроцессоры в виде #if'ок нужны для того что сука на линуксе и маке либа по другому называется
| { | ||
| var clazz = obj->decodeClass(); | ||
|
|
||
| IshtarSync.EnterCriticalSection(ref clazz.Owner.Interlocker.GC_FINALIZER_BARRIER); |
There was a problem hiding this comment.
при каждой аллокации объекта фриз?
There was a problem hiding this comment.
Ну объект уже заалоцированный, в данной функции регистрирую финализатор, когда гц пернет желанием убить объект, вызовем юзер код если он есть и очистим память
EnterCriticalSection пока никак не используется, просто затычка
Это на будущее когда будет поточность будет
No description provided.