Skip to content
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

Initial Threading #236

Merged
merged 13 commits into from
Jun 10, 2024
Merged

Initial Threading #236

merged 13 commits into from
Jun 10, 2024

Conversation

0xF6
Copy link
Member

@0xF6 0xF6 commented Jun 10, 2024

Summary by CodeRabbit

  • New Features

    • Introduced a C# wrapper for libuv, providing bindings for various functions including thread creation and event loop management.
    • Added thread-safe NativeConcurrentDictionary and NativeQueue implementations.
    • Introduced IshtarRawThread, IshtarTask, IshtarThread, IshtarThreading, and TaskScheduler for advanced threading and task management.
  • Enhancements

    • Added Async flag to method options, expanding method flag capabilities.
    • Added multiple new methods and structs for thread and memory management in garbage collection.
  • Bug Fixes

    • Removed outdated and commented-out methods to improve code clarity and maintenance.
  • Tests

    • Added a new method in unit tests to create and manipulate native queues, ensuring robustness and correctness of the new queue functionality.
  • Chores

    • Updated project files and solution configurations to include new projects and adjust project types.

@0xF6 0xF6 linked an issue Jun 10, 2024 that may be closed by this pull request
1 task
Copy link
Contributor

coderabbitai bot commented Jun 10, 2024

Walkthrough

The recent updates introduce several enhancements and new features across the runtime and collections modules. Notable changes include the addition of an Async flag to the MethodFlags enum, the integration of a C# wrapper for libuv, and the introduction of various threading and task scheduling functionalities. The updates also include improvements to the garbage collection layout and new data structures like NativeConcurrentDictionary and NativeQueue.

Changes

File(s) / Path(s) Change Summary
runtime/common/reflection/MethodFlags.cs Added Async flag to MethodFlags enum.
runtime/ishtar.vm.libuv/LibUV.cs, .../ishtar.vm.libuv.csproj Introduced C# wrapper for libuv with bindings for various functions and a new project file.
runtime/ishtar.vm/FFI/ForeignFunctionInterface.cs, .../@llmv/LLVMContext.cs Renamed Compile21FFI method to CompileFFI.
runtime/ishtar.vm/Program.cs, .../VirtualMachine.cs Removed BoehmGCLayout.Native references, updated VirtualMachine initialization and flow.
runtime/ishtar.vm/collections/NativeConcurrentDictionary.cs Introduced NativeConcurrentDictionary struct for thread-safe dictionary operations.
runtime/ishtar.vm/collections/NativeQueue.cs Introduced NativeQueue struct for native queue operations.
runtime/ishtar.vm/collections/NativeDictionary.cs Removed a commented-out method GetValues().
runtime/ishtar.vm/runtime/NativeException.cs Added SEMAPHORE_FAILED to WNE enum.
runtime/ishtar.vm/runtime/gc/BoehmGCLayout.cs, .../GCLayout.cs, .../IshtarGC.cs Added new methods for thread management and stack base retrieval.
runtime/ishtar.vm/runtime/io/IshtarRawThread.cs, .../IshtarThread.cs Introduced IshtarRawThread struct and updated IshtarThread struct.
runtime/ishtar.vm/runtime/io/IshtarThreading.cs, .../TaskScheduler.cs Introduced structs and methods for thread and task management using LibUV.
runtime/ishtar.vm/runtime/vm/RuntimeIshtarMethod.cs Added ForceSetAsAsync() method to RuntimeIshtarMethod struct.
test/ishtar_collections_test/UnitTest1.cs Added method CreateNativeQueueAndFill for testing native queue operations.
vein_lang.sln, vein_lang.sln.DotSettings Updated solution file with new project type and added new project for ishtar.vm.libuv.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant VirtualMachine
    participant TaskScheduler
    participant LibUV

    User->>VirtualMachine: Start Method Execution
    VirtualMachine->>TaskScheduler: Schedule Task
    TaskScheduler->>LibUV: Initialize Event Loop
    TaskScheduler->>LibUV: Create Thread
    LibUV->>TaskScheduler: Thread Created
    TaskScheduler->>VirtualMachine: Task Scheduled
    VirtualMachine->>User: Method Execution Completed
Loading

Poem

In the code where threads now spin,
Async flags and tasks begin,
With libuv's embrace so tight,
Methods flow through day and night.
Concurrent queues and dictionaries too,
A rabbit's dream, all fresh and new. 🐇✨


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@0xF6 0xF6 added area-vm Area of virtual machine optimization priority 1 important area-runtime Area of runtime forge feature area-gc Area of garbage collector size: S small task (< 1 week) area-infrastructure labels Jun 10, 2024
@0xF6 0xF6 requested a review from code-maid June 10, 2024 17:54
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 20

Outside diff range and nitpick comments (4)
runtime/ishtar.vm/runtime/vm/RuntimeIshtarMethod.cs (1)

129-132: Add documentation for ForceSetAsAsync.

Consider adding a summary comment above the ForceSetAsAsync method to explain its purpose and usage, especially since it modifies the method's flags which could have significant implications in a multi-threaded environment.

runtime/ishtar.vm/runtime/gc/BoehmGCLayout.cs (1)

Line range hint 24-132: Ensure robust error handling in new GC methods.

The new methods for thread management and garbage collection are critical and should include comprehensive error handling to manage potential failures from the native library calls. Consider wrapping these calls in try-catch blocks and logging any exceptions that occur.

runtime/ishtar.vm/runtime/gc/IshtarGC.cs (2)

Line range hint 609-640: Optimize memory allocation methods.

Consider refactoring the memory allocation methods to reduce redundancy and improve performance. For example, you could create a generic method to handle all types of collections.

- public static AtomicNativeList<T>* AllocateAtomicList<T>(int initialCapacity = 16)
-     where T : unmanaged, IEquatable<T>
-     => AtomicNativeList<T>.Create(initialCapacity, _allocator);
- public static NativeDictionary<TKey, TValue>* AllocateDictionary<TKey, TValue>(int initialCapacity = 16)
-     where TKey : unmanaged, IEquatable<TKey> where TValue : unmanaged
-     => NativeDictionary<TKey, TValue>.Create(initialCapacity, _allocator);
- public static AtomicNativeDictionary<TKey, TValue>* AllocateAtomicDictionary<TKey, TValue>(int initialCapacity = 16)
-     where TKey : unmanaged, IEquatable<TKey> where TValue : unmanaged, IEquatable<TValue>
-     => AtomicNativeDictionary<TKey, TValue>.Create(initialCapacity, _allocator);
+ public static T* AllocateCollection<T>(int initialCapacity = 16) where T : unmanaged, ICollection
+     => T.Create(initialCapacity, _allocator);

625-630: Ensure proper disposal of queues.

The method FreeQueue should ensure that all elements in the queue are properly disposed of before freeing the queue itself to prevent resource leaks.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6edf728 and a6e2fa3.

Files selected for processing (23)
  • runtime/common/reflection/MethodFlags.cs (1 hunks)
  • runtime/ishtar.vm.libuv/LibUV.cs (1 hunks)
  • runtime/ishtar.vm.libuv/ishtar.vm.libuv.csproj (1 hunks)
  • runtime/ishtar.vm/FFI/ForeignFunctionInterface.cs (1 hunks)
  • runtime/ishtar.vm/Program.cs (4 hunks)
  • runtime/ishtar.vm/VirtualMachine.cs (5 hunks)
  • runtime/ishtar.vm/collections/NativeConcurrentDictionary.cs (1 hunks)
  • runtime/ishtar.vm/collections/NativeDictionary.cs (1 hunks)
  • runtime/ishtar.vm/collections/NativeQueue.cs (1 hunks)
  • runtime/ishtar.vm/runtime/NativeException.cs (1 hunks)
  • runtime/ishtar.vm/runtime/gc/BoehmGCLayout.cs (3 hunks)
  • runtime/ishtar.vm/runtime/gc/GCLayout.cs (2 hunks)
  • runtime/ishtar.vm/runtime/gc/IshtarGC.cs (4 hunks)
  • runtime/ishtar.vm/runtime/io/IshtarRawThread.cs (1 hunks)
  • runtime/ishtar.vm/runtime/io/IshtarTask.cs (1 hunks)
  • runtime/ishtar.vm/runtime/io/IshtarThread.cs (1 hunks)
  • runtime/ishtar.vm/runtime/io/IshtarThreading.cs (1 hunks)
  • runtime/ishtar.vm/runtime/io/TaskScheduler.cs (1 hunks)
  • runtime/ishtar.vm/runtime/jit/@llmv/LLVMContext.cs (1 hunks)
  • runtime/ishtar.vm/runtime/vm/RuntimeIshtarMethod.cs (1 hunks)
  • test/ishtar_collections_test/UnitTest1.cs (1 hunks)
  • vein_lang.sln (3 hunks)
  • vein_lang.sln.DotSettings (1 hunks)
Files skipped from review due to trivial changes (5)
  • runtime/ishtar.vm.libuv/ishtar.vm.libuv.csproj
  • runtime/ishtar.vm/FFI/ForeignFunctionInterface.cs
  • runtime/ishtar.vm/collections/NativeDictionary.cs
  • runtime/ishtar.vm/runtime/jit/@llmv/LLVMContext.cs
  • vein_lang.sln.DotSettings
Additional context used
GitHub Check: Codacy Static Code Analysis
runtime/ishtar.vm/runtime/io/IshtarThread.cs

[warning] 5-5: runtime/ishtar.vm/runtime/io/IshtarThread.cs#L5
Implement 'IEquatable' in value type 'IshtarThread'.

runtime/ishtar.vm/runtime/io/IshtarRawThread.cs

[warning] 6-6: runtime/ishtar.vm/runtime/io/IshtarRawThread.cs#L6
Implement 'IEquatable' in value type 'IshtarRawThread'.

runtime/ishtar.vm/runtime/io/IshtarTask.cs

[warning] 7-7: runtime/ishtar.vm/runtime/io/IshtarTask.cs#L7
Implement 'IEquatable' in value type 'IshtarTask'.


[warning] 16-16: runtime/ishtar.vm/runtime/io/IshtarTask.cs#L16
Implement 'IEquatable' in value type 'TaskData'.

runtime/ishtar.vm/runtime/gc/GCLayout.cs

[warning] 3-3: runtime/ishtar.vm/runtime/gc/GCLayout.cs#L3
Implement 'IEquatable' in value type 'GC_stack_base'.

runtime/ishtar.vm/runtime/io/IshtarThreading.cs

[warning] 7-7: runtime/ishtar.vm/runtime/io/IshtarThreading.cs#L7
Implement 'IEquatable' in value type 'IshtarThreading'.

runtime/ishtar.vm/runtime/io/TaskScheduler.cs

[warning] 9-9: runtime/ishtar.vm/runtime/io/TaskScheduler.cs#L9
Implement 'IEquatable' in value type 'TaskScheduler'.


[warning] 101-101: runtime/ishtar.vm/runtime/io/TaskScheduler.cs#L101
Implement 'IEquatable' in value type 'Data'.

runtime/ishtar.vm/collections/NativeQueue.cs

[warning] 5-5: runtime/ishtar.vm/collections/NativeQueue.cs#L5
Implement 'IEquatable' in value type 'NativeQueue'.

runtime/ishtar.vm.libuv/LibUV.cs

[warning] 89-89: runtime/ishtar.vm.libuv/LibUV.cs#L89
Implement 'IEquatable' in value type 'uv_thread_t'.


[warning] 95-95: runtime/ishtar.vm.libuv/LibUV.cs#L95
Implement 'IEquatable' in value type 'uv_sem_t'.


[warning] 101-101: runtime/ishtar.vm.libuv/LibUV.cs#L101
Implement 'IEquatable' in value type 'uv_work_t'.

runtime/ishtar.vm/collections/NativeConcurrentDictionary.cs

[warning] 5-5: runtime/ishtar.vm/collections/NativeConcurrentDictionary.cs#L5
Implement 'IEquatable' in value type 'NativeConcurrentDictionary'.


[warning] 31-31: runtime/ishtar.vm/collections/NativeConcurrentDictionary.cs#L31
Implement 'IEquatable' in value type 'sem_slim'.

Additional comments not posted (14)
runtime/common/reflection/MethodFlags.cs (1)

18-19: Addition of Async flag aligns well with the PR's focus on threading and asynchronous execution.

The Async flag is a necessary addition for supporting asynchronous method declarations, enhancing the language's capabilities in concurrent environments.

runtime/ishtar.vm/runtime/NativeException.cs (1)

34-34: The addition of SEMAPHORE_FAILED to the WNE enum is appropriate for enhanced error handling in threading operations.

runtime/ishtar.vm/Program.cs (1)

Line range hint 16-69: The changes in the main execution flow of the virtual machine, including the use of ForceSetAsAsync and the integration of a task scheduler, align well with the PR's objectives to enhance threading capabilities.

runtime/ishtar.vm/runtime/io/TaskScheduler.cs (2)

76-77: Ensure that the loop is properly managed to prevent deadlocks or unresponsive behavior.


43-49: Ensure that method flags are correctly set for asynchronous execution to prevent runtime errors.

runtime/ishtar.vm.libuv/LibUV.cs (1)

10-66: Ensure that all P/Invoke declarations are correct and that the library path and calling conventions are properly specified to prevent runtime errors.

test/ishtar_collections_test/UnitTest1.cs (1)

87-144: Enhance clarity and robustness of CreateNativeQueueAndFill.
[REFACTOR_SUGGESTion]

  • Consider adding comments within the test method to explain the purpose of each block of code, especially the conditions under which garbage collection is invoked.
  • It might be beneficial to split this large test method into smaller, more focused tests that each cover a specific aspect of the queue's functionality.
vein_lang.sln (3)

58-58: The addition of the ishtar_collections_test project with the updated project GUID {9A19103F-16F7-4668-BE54-9A1E7A4F7556} aligns with the new project structure. Ensure that all references and dependencies are correctly configured.


65-66: The addition of the ishtar.vm.libuv project is crucial for the integration of LibUV functionalities. Ensure that the project settings and dependencies are correctly configured to support the new threading and task management features.


281-292: The configuration settings for the ishtar.vm.libuv project across different platforms (Any CPU, x64, x86) have been correctly set up for both Debug and Release modes. This ensures proper build configurations are in place for the new functionalities.

runtime/ishtar.vm/runtime/gc/IshtarGC.cs (1)

609-610: Validate the use of generics with unmanaged constraint.

Verification successful

The search results indicate that AllocateAtomicList is used in two places:

  • runtime/ishtar.vm/runtime/vm/RuntimeIshtarModule.cs at line 936
  • runtime/ishtar.vm/runtime/gc/IshtarGC.cs at line 609

To ensure that only unmanaged types are passed as arguments to AllocateAtomicList, we need to inspect the type T used in these instances.


The search results show the usage of AllocateAtomicList<T> in RuntimeIshtarModule.cs but do not provide enough context to determine the type of T. To ensure that T is an unmanaged type, we need to trace back the definition or usage context of T in RuntimeIshtarModule.cs.


The search results show multiple instances of T in RuntimeIshtarModule.cs, but they do not provide enough context to determine the type of T used in the specific call to AllocateAtomicList<T>. To ensure that T is an unmanaged type, we need to trace back the definition or usage context of T in RuntimeIshtarModule.cs.


The search results show that the AllocateAtomicList<T> method is used within the ToNative<T> method in RuntimeIshtarModule.cs. The ToNative<T> method has the constraint where T : unmanaged, IEquatable<T>, which ensures that T is an unmanaged type.

Therefore, the usage of generics with the unmanaged constraint in the AllocateAtomicList method is correctly handled.

  • runtime/ishtar.vm/runtime/vm/RuntimeIshtarModule.cs at line 936: AllocateAtomicList<T> is used within ToNative<T> with the constraint where T : unmanaged, IEquatable<T>.
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that all usages of `AllocateAtomicList` correctly handle unmanaged types.

# Test: Search for usages of `AllocateAtomicList`. Expect: Only unmanaged types are passed as arguments.
ast-grep --lang csharp --pattern $'AllocateAtomicList<$_>(...)' 

Length of output: 409



Script:

#!/bin/bash
# Description: Verify that all usages of `AllocateAtomicList` correctly handle unmanaged types.

# Test: Search for usages of `AllocateAtomicList`. Expect: Only unmanaged types are passed as arguments.
ast-grep --lang csharp --pattern $'AllocateAtomicList<$T>(...)' 

Length of output: 409



Script:

#!/bin/bash
# Description: Verify that all usages of `AllocateAtomicList` correctly handle unmanaged types.

# Test: Search for usages of `AllocateAtomicList`. Expect: Only unmanaged types are passed as arguments.
rg --no-heading --line-number --color=always 'AllocateAtomicList<[^>]+>'

Length of output: 414



Script:

#!/bin/bash
# Description: Verify that the type `T` used in `AllocateAtomicList` is unmanaged.

# Extract the type `T` used in `AllocateAtomicList` in `RuntimeIshtarModule.cs`
rg --no-heading --line-number --color=always 'var t = IshtarGC.AllocateAtomicList<[^>]+>' runtime/ishtar.vm/runtime/vm/RuntimeIshtarModule.cs -A 5

# Extract the type `T` used in `AllocateAtomicList` in `IshtarGC.cs`
rg --no-heading --line-number --color=always 'public static AtomicNativeList<T>* AllocateAtomicList<T>' runtime/ishtar.vm/runtime/gc/IshtarGC.cs -A 5

Length of output: 551



Script:

#!/bin/bash
# Description: Trace back the definition or usage context of `T` in `RuntimeIshtarModule.cs` to ensure it is an unmanaged type.

# Search for the definition or context of `T` in `RuntimeIshtarModule.cs`
rg --no-heading --line-number --color=always 'T' runtime/ishtar.vm/runtime/vm/RuntimeIshtarModule.cs -A 10 -B 10

Length of output: 56547



Script:

#!/bin/bash
# Description: Trace back the definition or usage context of `T` in `RuntimeIshtarModule.cs` to ensure it is an unmanaged type.

# Search for the definition or context of `T` in `RuntimeIshtarModule.cs`
rg --no-heading --line-number --color=always 'AllocateAtomicList<T>' runtime/ishtar.vm/runtime/vm/RuntimeIshtarModule.cs -A 10 -B 10

Length of output: 1130

runtime/ishtar.vm/VirtualMachine.cs (3)

28-32: Integrate garbage collection with threading support.

The integration of BoehmGC with threading by calling GC_allow_register_threads() ensures that the garbage collector is aware of multiple threads, which is crucial for correct memory management in a multi-threaded environment.


56-58: Initialize threading and task scheduling.

The creation of a new IshtarThreading instance and a task scheduler is a critical part of setting up the threading infrastructure. This setup is essential for managing asynchronous tasks and threads efficiently.


732-735: Handle external and internal method executions differently.

The distinction between executing external native methods and scheduling tasks for internal methods is well-handled. This separation ensures that each type of method is managed according to its specific requirements and execution context.

Comment on lines +552 to +554
public void register_thread(GC_stack_base* attr) => gcLayout.register_thread(attr);
public void unregister_thread() => gcLayout.unregister_thread();
public bool get_stack_base(GC_stack_base* attr) => gcLayout.get_stack_base(attr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure proper error handling in threading methods.

The methods register_thread, unregister_thread, and get_stack_base lack error handling. Consider adding checks for null pointers and appropriate exception handling to prevent crashes in edge cases.

Comment on lines +633 to +639
public static NativeConcurrentDictionary<TKey, TValue>* AllocateConcurrentDictionary<TKey, TValue>(int initialCapacity = 16)
where TKey : unmanaged, IEquatable<TKey> where TValue : unmanaged
=> NativeConcurrentDictionary<TKey, TValue>.Create(initialCapacity, _allocator);

public static void FreeConcurrentDictionary<TKey, TValue>(NativeConcurrentDictionary<TKey, TValue>* list)
where TKey : unmanaged, IEquatable<TKey> where TValue : unmanaged
=> NativeConcurrentDictionary<TKey, TValue>.Free(list, _allocator);
Copy link
Contributor

Choose a reason for hiding this comment

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

Review memory management for concurrent collections.

The methods AllocateConcurrentDictionary and FreeConcurrentDictionary manage memory for concurrent dictionaries. Ensure that the memory allocated for these dictionaries is properly freed to avoid memory leaks.

Comment on lines +5 to +10
public readonly unsafe struct IshtarThread(
uv_thread_t threadId,
CallFrame* frame)
{
public IshtarThread* CreateThread() => throw new NotImplementedException();
public int GetExitCodeThread(IshtarThread* thread) => throw new NotImplementedException();
public void ExitThread(IshtarThread* thread, int exitCode) => throw new NotImplementedException();
public void Sleep(IshtarThread* thread, uint ms) => throw new NotImplementedException();
public readonly uv_thread_t threadId = threadId;
public readonly CallFrame* callFrame = frame;
Copy link
Contributor

Choose a reason for hiding this comment

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

Implement IEquatable<IshtarThread> for better performance and correctness.

Implementing IEquatable<IshtarThread> will optimize equality checks, especially useful in collections or when instances are used as keys in dictionaries.

Tools
GitHub Check: Codacy Static Code Analysis

[warning] 5-5: runtime/ishtar.vm/runtime/io/IshtarThread.cs#L5
Implement 'IEquatable' in value type 'IshtarThread'.

Comment on lines +6 to +14
public readonly unsafe struct IshtarRawThread(
LibUV.uv_thread_t threadId,
delegate*<nint, void> frame,
RuntimeIshtarModule* mainModule)
{
public readonly RuntimeIshtarModule* MainModule = mainModule;
public readonly LibUV.uv_thread_t threadId = threadId;
public readonly delegate*<nint, void> callFrame = frame;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Implement IEquatable<IshtarRawThread> for better performance and correctness.

Implementing IEquatable<IshtarRawThread> will optimize equality checks, especially useful in collections or when instances are used as keys in dictionaries.

Tools
GitHub Check: Codacy Static Code Analysis

[warning] 6-6: runtime/ishtar.vm/runtime/io/IshtarRawThread.cs#L6
Implement 'IEquatable' in value type 'IshtarRawThread'.

Comment on lines +7 to +22
public readonly unsafe struct IshtarTask(CallFrame* frame, ulong index) : IEq<IshtarTask>, IDisposable
{
public readonly ulong Index = index;
public readonly TaskData* Data = IshtarGC.AllocateImmortal<TaskData>();
public readonly CallFrame* Frame = frame;

public static bool Eq(IshtarTask* p1, IshtarTask* p2) => false;


public struct TaskData
{
public LibUV.uv_sem_t semaphore;
}

public void Dispose() => IshtarGC.FreeImmortal(Data);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Implement IEquatable<IshtarTask> and IEquatable<TaskData> for better performance and correctness.

Implementing IEquatable<IshtarTask> and IEquatable<TaskData> will optimize equality checks, especially useful in collections or when instances are used as keys in dictionaries. This is particularly important for IshtarTask given its role in task management.

Tools
GitHub Check: Codacy Static Code Analysis

[warning] 7-7: runtime/ishtar.vm/runtime/io/IshtarTask.cs#L7
Implement 'IEquatable' in value type 'IshtarTask'.


[warning] 16-16: runtime/ishtar.vm/runtime/io/IshtarTask.cs#L16
Implement 'IEquatable' in value type 'TaskData'.

Comment on lines +37 to +54
public void Enqueue(T* value)
{
if (count == capacity)
EnsureCapacity(capacity * 2);
items[tail] = value;
tail = (tail + 1) % capacity;
count++;
}

public T* Dequeue()
{
if (count == 0)
throw new InvalidOperationException("Queue is empty.");
T* value = items[head];
head = (head + 1) % capacity;
count--;
return value;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure that the queue capacity management is robust to prevent overflow and ensure thread safety.

+ if (count == capacity) {
+     EnsureCapacity(capacity * 2);
+ }

Committable suggestion was skipped due to low confidence.

Comment on lines +89 to +104
public struct uv_thread_t
{
private nint handle;
}

[StructLayout(LayoutKind.Sequential)]
public struct uv_sem_t
{
private nint handle;
}

[StructLayout(LayoutKind.Sequential)]
public struct uv_work_t
{
private nint handle;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider implementing IEquatable<T> for uv_thread_t, uv_sem_t, and uv_work_t to ensure proper value comparison.

Tools
GitHub Check: Codacy Static Code Analysis

[warning] 89-89: runtime/ishtar.vm.libuv/LibUV.cs#L89
Implement 'IEquatable' in value type 'uv_thread_t'.


[warning] 95-95: runtime/ishtar.vm.libuv/LibUV.cs#L95
Implement 'IEquatable' in value type 'uv_sem_t'.


[warning] 101-101: runtime/ishtar.vm.libuv/LibUV.cs#L101
Implement 'IEquatable' in value type 'uv_work_t'.


using static vm.libuv.LibUV;

public unsafe struct NativeConcurrentDictionary<TKey, TValue> where TKey : unmanaged, IEquatable<TKey> where TValue : unmanaged
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider implementing IEquatable<NativeConcurrentDictionary> to ensure proper value comparison, especially useful in collections or when sorting.

Tools
GitHub Check: Codacy Static Code Analysis

[warning] 5-5: runtime/ishtar.vm/collections/NativeConcurrentDictionary.cs#L5
Implement 'IEquatable' in value type 'NativeConcurrentDictionary'.

Comment on lines +160 to +169
public void Clear()
{
using var locker = new sem_slim(_self);
_allocator.free(keys);
_allocator.free(values);
capacity = 16;
keys = (TKey*)_allocator.alloc((uint)(capacity * sizeof(TKey)));
values = (TValue**)_allocator.alloc((uint)(capacity * sizeof(TValue*)));
count = 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure that all resources are properly released when clearing the dictionary to prevent memory leaks.

Comment on lines +61 to +136
public void Add(TKey key, TValue* value)
{
if (!TryAdd(key, value))
throw new ArgumentException("An element with the same key already exists in the dictionary.");
}

public bool TryAdd(TKey key, TValue* value)
{
using var locker = new sem_slim(_self);
if (FindIndex(key) != -1) // do not use ContainsKey
return false;
if (count == capacity)
Resize();

keys[count] = key;
values[count] = value;
count++;
return true;
}

private void Resize()
{
capacity *= 2;

keys = (TKey*)_allocator.realloc(keys, (uint)(capacity * sizeof(TKey)));
values = (TValue**)_allocator.realloc(values, (uint)(capacity * sizeof(TValue*)));
}

public void Remove(TKey key)
{
if (!TryRemove(key))
throw new ArgumentException("Key not found in the dictionary.");
}

public bool TryRemove(TKey key)
{
using var locker = new sem_slim(_self);

int index = FindIndex(key);
if (index == -1)
return false;

keys[index] = keys[count - 1];
values[index] = values[count - 1];
count--;
return true;
}

public bool ContainsKey(TKey key)
{
using var locker = new sem_slim(_self);
return FindIndex(key) != -1;
}

public TValue* Get(TKey key)
{
using var locker = new sem_slim(_self);
int index = FindIndex(key);
if (index == -1)
throw new ArgumentException("Key not found in the dictionary.");
return values[index];
}

public bool TryGetValue(TKey key, out TValue* value)
{
using var locker = new sem_slim(_self);
int index = FindIndex(key);
if (index == -1)
{
value = null;
return false;
}

value = values[index];
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure that dictionary operations are thread-safe and that capacity management is robust to prevent data corruption or performance issues.

+ if (count == capacity) {
+     Resize();
+ }

Committable suggestion was skipped due to low confidence.

@0xF6 0xF6 merged commit 686032d into master Jun 10, 2024
15 of 17 checks passed
@0xF6 0xF6 deleted the feature/threading branch June 10, 2024 20:05
@vein-lang vein-lang locked and limited conversation to collaborators Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-gc Area of garbage collector area-infrastructure area-runtime Area of runtime forge area-vm Area of virtual machine feature optimization priority 1 important size: S small task (< 1 week)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EPIC] Threading
1 participant