Threading bindings #256
Threading bindings #256
Conversation
0xF6
added
area-vm
WalkthroughThe changes introduce a comprehensive update to threading management in the Ishtar virtual machine's runtime. Enhancements include the addition of several new methods for thread creation, management, and synchronization, along with structural changes to relevant classes and functions. These updates improve thread handling, ensuring better performance and accuracy in multithreaded scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant B_Threading
participant TaskScheduler
participant IshtarThreading
participant IshtarJob
User->>+B_Threading: createThread(current, args)
B_Threading->>+TaskScheduler: Execute Task
TaskScheduler->>+IshtarThreading: CreateRawThread(module, frame, name)
IshtarThreading->>-TaskScheduler: Return IshtarThread
TaskScheduler->>-B_Threading: Task Enqueued
B_Threading->>-User: Thread Created
Note over User,B_Threading: Thread is running
User->>+B_Threading: join(current, args)
B_Threading->>+IshtarThreading: DestroyThread(thread)
IshtarThreading->>-B_Threading: Thread Destroyed
B_Threading->>-User: Thread Joined
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 review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Outside diff range and nitpick comments (4)
runtime/ishtar.vm/runtime/io/IshtarThreading.cs (4)
6-6: Remove unused import.The
using static libuv.LibUV;import is not used in the provided code.- using static libuv.LibUV;
30-30: Remove debug print statement.Consider removing or wrapping the debug print statement in a debug flag to avoid unnecessary console output in production.
#if DEBUG frame->vm.println($"thread start {threadId}"); #endif
52-54: Remove debug print statements.Consider removing or wrapping the debug print statements in a debug flag to avoid unnecessary console output in production.
#if DEBUG Thread.CurrentThread.Name = $"Thread [{threadData->callFrame->method->Name}]"; #endif
81-81: Remove debug print statements.Consider removing or wrapping the debug print statements in a debug flag to avoid unnecessary console output in production.
#if DEBUG Thread.CurrentThread.Name = StringStorage.GetStringUnsafe(thread->Name); #endif
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- runtime/ishtar.vm/__builtin/B_Threading.cs (1 hunks)
- runtime/ishtar.vm/runtime/io/IshtarRawThread.cs (1 hunks)
- runtime/ishtar.vm/runtime/io/IshtarThread.cs (3 hunks)
- runtime/ishtar.vm/runtime/io/IshtarThreading.cs (2 hunks)
- runtime/ishtar.vm/runtime/io/TaskScheduler.cs (2 hunks)
Additional context used
GitHub Check: Codacy Static Code Analysis
runtime/ishtar.vm/runtime/io/IshtarThread.cs
[warning] 67-67: runtime/ishtar.vm/runtime/io/IshtarThread.cs#L67
Implement 'IEquatable' in value type 'IshtarJobContext'.
[warning] 77-77: runtime/ishtar.vm/runtime/io/IshtarThread.cs#L77
Implement 'IEquatable' in value type 'IshtarJob'.
Additional comments not posted (2)
runtime/ishtar.vm/runtime/io/IshtarRawThread.cs (1)
15-15: LGTM!The addition of the
Namefield is appropriate and aligns with the changes in the constructor.runtime/ishtar.vm/runtime/io/TaskScheduler.cs (1)
119-119: LGTM! Thread naming is a good practice.The
CreateRawThreadcall now includes astring nameparameter, which is beneficial for debugging and monitoring.
| @@ -6,9 +6,11 @@ namespace ishtar.io; | |||
| public readonly unsafe struct IshtarRawThread( | |||
| LibUV.uv_thread_t threadId, | |||
| delegate*<IshtarRawThread*, void> frame, | |||
| RuntimeIshtarModule* mainModule) | |||
| RuntimeIshtarModule* mainModule, | |||
| InternedString* name) | |||
There was a problem hiding this comment.
Add null check for the name parameter.
To ensure robustness, consider adding a null check for the name parameter in the constructor.
if (name == null)
throw new ArgumentNullException(nameof(name));| { | ||
| public IshtarRawThread* CreateRawThread(RuntimeIshtarModule* mainModule, delegate*<IshtarRawThread*, void> frame) | ||
| public NativeList<IshtarThread>* threads = IshtarGC.AllocateList<IshtarThread>(vm.@ref); |
There was a problem hiding this comment.
Consider lazy initialization for threads field.
Lazy initialization can improve performance by delaying the creation of the threads list until it is actually needed.
public NativeList<IshtarThread>* threads;
public IshtarThreading(VirtualMachine vm)
{
this.vm = vm;
threads = null;
}
public NativeList<IshtarThread>* GetThreads()
{
if (threads == null)
threads = IshtarGC.AllocateList<IshtarThread>(vm.@ref);
return threads;
}| LibUV.uv_thread_create(out var threadId, executeRaw, (IntPtr)thread); | ||
| *thread = new IshtarRawThread(threadId, frame, mainModule); | ||
| uv_thread_create(out var threadId, executeRaw, (IntPtr)thread); | ||
| *thread = new IshtarRawThread(threadId, frame, mainModule, StringStorage.Intern(name, mainModule)); |
There was a problem hiding this comment.
Handle uv_thread_create error.
Check the return value of uv_thread_create and handle any errors appropriately.
var result = uv_thread_create(out var threadId, executeRaw, (IntPtr)thread);
if (result != 0)
throw new InvalidOperationException($"Failed to create thread: {result}");|
|
||
| uv_thread_create(out var threadId, execute, (IntPtr)thread); |
There was a problem hiding this comment.
Handle uv_thread_create error.
Check the return value of uv_thread_create and handle any errors appropriately.
var result = uv_thread_create(out var threadId, execute, (IntPtr)thread);
if (result != 0)
throw new InvalidOperationException($"Failed to create thread: {result}");| return thread; | ||
| } | ||
|
|
||
| public static void DestroyThread(IshtarThread* thread) | ||
| { | ||
| thread->callFrame->vm.println($"[thread] exit thread with status {thread->ctx->Status} {thread->threadId}"); |
There was a problem hiding this comment.
Handle uv_sem_destroy error.
Check the return value of uv_sem_destroy and handle any errors appropriately.
var result = uv_sem_destroy(ref thread->ctx->Locker);
if (result != 0)
throw new InvalidOperationException($"Failed to destroy semaphore: {result}");| var thread = getThread(current, args); | ||
| thread->join(); |
There was a problem hiding this comment.
Handle potential null values in join.
Add a null check for thread before calling join.
if (thread == null)
throw new InvalidOperationException("Thread is null.");
thread->join();| var thread = getThread(current, args); | ||
|
|
||
| thread->start(); |
There was a problem hiding this comment.
Handle potential null values in start.
Add a null check for thread before calling start.
if (thread == null)
throw new InvalidOperationException("Thread is null.");
thread->start();| var err = uv_thread_join(threadId); | ||
| VirtualMachine.Assert(err == 0, WNE.THREAD_STATE_CORRUPTED, | ||
| $"uv_thread_join returned error status, code: {err}"); |
There was a problem hiding this comment.
Handle uv_thread_join error.
Check the return value of uv_thread_join and handle any errors appropriately.
var err = uv_thread_join(threadId);
if (err != 0)
throw new InvalidOperationException($"uv_thread_join returned error status, code: {err}");| { | ||
| CREATED, | ||
| RUNNING, | ||
| PAUSED, | ||
| CANCELED, | ||
| EXITED | ||
| } |
There was a problem hiding this comment.
Implement IEquatable<T> for IshtarJobContext.
Implementing IEquatable<T> can improve performance and ensure consistent equality checks.
public struct IshtarJobContext : IEquatable<IshtarJobContext>
{
// Existing fields...
public bool Equals(IshtarJobContext other)
{
return ThreadId.Equals(other.ThreadId) && Locker.Equals(other.Locker);
}
public override bool Equals(object obj)
{
return obj is IshtarJobContext other && Equals(other);
}
public override int GetHashCode()
{
return HashCode.Combine(ThreadId, Locker);
}
}| public readonly unsafe struct IshtarJob(uv_work_t* workerId, CallFrame* frame, IshtarJobContext* ctx) : IEq<IshtarJob> | ||
| { | ||
| public readonly IshtarJobContext* ctx = ctx; | ||
| public readonly uv_work_t* workerId = workerId; | ||
| public readonly CallFrame* callFrame = frame; | ||
|
|
||
| public void start() | ||
| { | ||
| VirtualMachine.Assert(ctx->Status == IshtarJobStatus.CREATED, WNE.THREAD_STATE_CORRUPTED, | ||
| "trying start worker with out of CREATED status"); | ||
| ctx->Status = IshtarJobStatus.RUNNING; | ||
| uv_sem_post(ref ctx->Locker); | ||
| } | ||
|
|
||
| public void complete() | ||
| => ctx->Status = IshtarJobStatus.EXITED; | ||
|
|
||
| public void cancel() | ||
| { | ||
| ctx->Status = IshtarJobStatus.CANCELED; | ||
| var err = uv_cancel(workerId); | ||
| VirtualMachine.Assert(err == 0, WNE.THREAD_STATE_CORRUPTED, | ||
| $"uv_cancel work_t returned error status, code: {err}"); | ||
| } | ||
|
|
||
| public void wait_until_complete_or_cancel() | ||
| { | ||
| begin: | ||
| if (ctx->Status is IshtarJobStatus.EXITED) | ||
| return; | ||
| if (ctx->Status is IshtarJobStatus.CANCELED) | ||
| return; | ||
|
|
||
| uv_sleep(1); | ||
| goto begin; | ||
| } | ||
|
|
||
|
|
||
| public static bool Eq(IshtarJob* p1, IshtarJob* p2) => p1->workerId->handle.Equals(p2->workerId->handle); | ||
| } |
There was a problem hiding this comment.
Implement IEquatable<T> for IshtarJob.
Implementing IEquatable<T> can improve performance and ensure consistent equality checks.
public readonly unsafe struct IshtarJob : IEquatable<IshtarJob>
{
// Existing fields...
public bool Equals(IshtarJob other)
{
return workerId->handle.Equals(other.workerId->handle);
}
public override bool Equals(object obj)
{
return obj is IshtarJob other && Equals(other);
}
public override int GetHashCode()
{
return workerId->handle.GetHashCode();
}
}Tools
GitHub Check: Codacy Static Code Analysis
[warning] 77-77: runtime/ishtar.vm/runtime/io/IshtarThread.cs#L77
Implement 'IEquatable' in value type 'IshtarJob'.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- runtime/ishtar.vm/__builtin/B_Threading.cs (1 hunks)
- runtime/ishtar.vm/runtime/io/IshtarRawThread.cs (1 hunks)
- runtime/ishtar.vm/runtime/io/IshtarThread.cs (3 hunks)
- runtime/ishtar.vm/runtime/io/IshtarThreading.cs (2 hunks)
- runtime/ishtar.vm/runtime/io/TaskScheduler.cs (2 hunks)
Additional context used
GitHub Check: Codacy Static Code Analysis
runtime/ishtar.vm/runtime/io/IshtarThread.cs
[warning] 67-67: runtime/ishtar.vm/runtime/io/IshtarThread.cs#L67
Implement 'IEquatable' in value type 'IshtarJobContext'.
[warning] 77-77: runtime/ishtar.vm/runtime/io/IshtarThread.cs#L77
Implement 'IEquatable' in value type 'IshtarJob'.
Additional comments not posted (23)
runtime/ishtar.vm/runtime/io/IshtarRawThread.cs (1)
9-10: LGTM!The inclusion of the
nameparameter and theNamefield in theIshtarRawThreadstruct is appropriate and aligns with the threading enhancements.Also applies to: 15-15
runtime/ishtar.vm/runtime/io/IshtarThreading.cs (6)
10-10: LGTM!The addition of the
threadsfield for managing thread lists in theIshtarThreadingstruct is appropriate and aligns with the threading enhancements.
12-16: LGTM!The inclusion of the
nameparameter in theCreateRawThreadmethod is appropriate and aligns with the threading enhancements.
18-33: LGTM!The
CreateThreadmethod is appropriate and aligns with the threading enhancements.
37-43: LGTM!The
DestroyThreadmethod is appropriate and aligns with the threading enhancements.
Line range hint
44-72:
LGTM!The
executemethod is appropriate and aligns with the threading enhancements.
73-83: LGTM!The
executeRawmethod is appropriate and aligns with the threading enhancements.runtime/ishtar.vm/__builtin/B_Threading.cs (6)
9-15: LGTM!The
getThreadClassmethod is appropriate and aligns with the threading enhancements.
17-43: LGTM!The updates to the
createThreadmethod are appropriate and align with the threading enhancements.
45-51: LGTM!The
sleepmethod is appropriate and aligns with the threading enhancements.
52-58: LGTM!The
getThreadmethod is appropriate and aligns with the threading enhancements.
59-65: LGTM!The
joinmethod is appropriate and aligns with the threading enhancements.
66-72: LGTM!The
startmethod is appropriate and aligns with the threading enhancements.runtime/ishtar.vm/runtime/io/IshtarThread.cs (9)
Line range hint
9-33:
LGTM!The modifications to the
IshtarThreadstruct, including the implementation of theIEqinterface and the addition of thecomplete,join, andstartmethods, are appropriate and align with the threading enhancements.
Line range hint
36-43:
LGTM!The addition of the
IshtarThreadContextstruct is appropriate and aligns with the threading enhancements.
48-56: LGTM!The addition of the
IshtarThreadStatusenum is appropriate and aligns with the threading enhancements.
58-65: LGTM!The addition of the
IshtarJobStatusenum is appropriate and aligns with the threading enhancements.
67-75: LGTM!The addition of the
IshtarJobContextstruct is appropriate and aligns with the threading enhancements.Tools
GitHub Check: Codacy Static Code Analysis
[warning] 67-67: runtime/ishtar.vm/runtime/io/IshtarThread.cs#L67
Implement 'IEquatable' in value type 'IshtarJobContext'.
77-116: LGTM!The addition of the
IshtarJobstruct and the implementation of theIEqinterface are appropriate and align with the threading enhancements.Tools
GitHub Check: Codacy Static Code Analysis
[warning] 77-77: runtime/ishtar.vm/runtime/io/IshtarThread.cs#L77
Implement 'IEquatable' in value type 'IshtarJob'.
83-89: LGTM!The
startmethod in theIshtarJobstruct is appropriate and aligns with the threading enhancements.
91-93: LGTM!The
completemethod in theIshtarJobstruct is appropriate and aligns with the threading enhancements.
94-112: LGTM!The
cancelandwait_until_complete_or_cancelmethods in theIshtarJobstruct are appropriate and align with the threading enhancements.runtime/ishtar.vm/runtime/io/TaskScheduler.cs (1)
119-119: LGTM! The addition of thread naming improves debugging.The inclusion of the string parameter for thread naming in the
CreateRawThreadcall enhances readability and debugging capabilities.
| public static void enqueue(CallFrame* method) | ||
| { | ||
| //uv_work_t t; | ||
| //uv_queue_work((nint)async_header, ref t) | ||
| } |
There was a problem hiding this comment.
Incomplete method: enqueue
The enqueue method is currently incomplete and contains commented-out code. Please provide the intended functionality or remove the method if it is not needed.
Summary by CodeRabbit
New Features
Improvements
Updates