From e7cbd8cc28fb37b7514117efe56e36deefbf1ed2 Mon Sep 17 00:00:00 2001 From: "Yukihiro \"Matz\" Matsumoto" Date: Wed, 8 Oct 2025 15:03:06 +0900 Subject: [PATCH] mruby-task: add gc protection and optimize task operations Implements dual-mechanism GC protection and optimizes task lookup using pointer arithmetic based on PicoRuby reference implementation. GC Protection: - Add mrb_gc_register/unregister to protect Task objects - Implement mrb_task_mark_all() to mark task contexts during GC - Store proc reference in mrb_task to prevent premature collection - Integrate marking into gc.c root_scan_phase Performance Optimizations: - Add MRB2TASK macro for O(1) context-to-task conversion - Optimize Task.current: O(n) queue search -> O(1) pointer arithmetic - Optimize Task.pass: simplify to root context check - Optimize Task.join: use MRB2TASK for current task lookup Bug Fixes: - Fix MRB_TASK_CREATED/STOPPED to use MRB_FIBER_TERMINATED - Add safety check to prevent execution of terminated tasks - Initialize callinfo PC to bytecode start in task_init_context Co-authored-by: Claude --- include/mruby/internal.h | 5 ++ mrbgems/mruby-task/include/task.h | 6 +- mrbgems/mruby-task/src/task.c | 110 ++++++++++++++++++++++++++---- src/gc.c | 10 +++ 4 files changed, 117 insertions(+), 14 deletions(-) diff --git a/include/mruby/internal.h b/include/mruby/internal.h index 1d752d131..3ec83fa3a 100644 --- a/include/mruby/internal.h +++ b/include/mruby/internal.h @@ -266,4 +266,9 @@ mrb_value mrb_bint_lcm(mrb_state *mrb, mrb_value x, mrb_value y); mrb_value mrb_bint_abs(mrb_state *mrb, mrb_value x); #endif +#ifdef MRB_USE_TASK_SCHEDULER +/* GC marking for task scheduler */ +void mrb_task_mark_all(mrb_state *mrb); +#endif + #endif /* MRUBY_INTERNAL_H */ diff --git a/mrbgems/mruby-task/include/task.h b/mrbgems/mruby-task/include/task.h index 082bb2319..9b313671b 100644 --- a/mrbgems/mruby-task/include/task.h +++ b/mrbgems/mruby-task/include/task.h @@ -53,6 +53,7 @@ typedef struct mrb_task { mrb_value self; /* Ruby Task object reference */ mrb_value result; /* Task return value */ + mrb_value proc; /* Proc containing task code */ struct mrb_context c; /* Execution context (stack, callinfo, etc) */ } mrb_task; @@ -95,12 +96,13 @@ void mrb_task_hal_idle_cpu(mrb_state *mrb); */ void mrb_tick(mrb_state *mrb); mrb_value mrb_tasks_run(mrb_state *mrb); +void mrb_task_mark_all(mrb_state *mrb); /* * Task context status values (extends mrb_fiber_state) */ -#define MRB_TASK_CREATED (MRB_FIBER_TRANSFERRED + 1) -#define MRB_TASK_STOPPED (MRB_FIBER_TRANSFERRED + 2) +#define MRB_TASK_CREATED (MRB_FIBER_TERMINATED + 1) +#define MRB_TASK_STOPPED (MRB_FIBER_TERMINATED + 2) #endif /* MRB_USE_TASK_SCHEDULER */ diff --git a/mrbgems/mruby-task/src/task.c b/mrbgems/mruby-task/src/task.c index e51559cc9..b8c84462d 100644 --- a/mrbgems/mruby-task/src/task.c +++ b/mrbgems/mruby-task/src/task.c @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -39,6 +40,9 @@ #define wakeup_tick_ (mrb->task.wakeup_tick) #define switching_ (mrb->task.switching) +/* Get task from current context using pointer arithmetic */ +#define MRB2TASK(mrb) ((mrb_task *)((uint8_t *)mrb->c - offsetof(mrb_task, c))) + /* * Task data type for GC */ @@ -47,6 +51,9 @@ mrb_task_free(mrb_state *mrb, void *ptr) { mrb_task *t = (mrb_task*)ptr; if (t) { + /* Unregister from GC protection */ + mrb_gc_unregister(mrb, t->self); + /* Free context resources */ if (t->c.stbase) { mrb_free(mrb, t->c.stbase); @@ -63,6 +70,67 @@ static const struct mrb_data_type mrb_task_type = { "Task", mrb_task_free, }; +/* + * GC marking function for all tasks + * Called from gc.c during root_scan_phase + */ +void +mrb_task_mark_all(mrb_state *mrb) +{ + int qi; + int task_count = 0; + for (qi = 0; qi < 4; qi++) { + mrb_task *t = mrb->task.queues[qi]; + while (t) { + task_count++; + struct mrb_context *c = &t->c; + mrb_callinfo *ci; + size_t i, e; + + /* Mark task's stack */ + if (c->stbase) { + if (c->ci) { + e = (c->ci->stack ? c->ci->stack - c->stbase : 0); + e += mrb_ci_nregs(c->ci); + } + else { + e = 0; + } + if (c->stbase + e > c->stend) e = c->stend - c->stbase; + for (i = 0; i < e; i++) { + mrb_value v = c->stbase[i]; + if (!mrb_immediate_p(v)) { + mrb_gc_mark(mrb, mrb_basic_ptr(v)); + } + } + } + + /* Mark call stack */ + if (c->cibase && c->ci) { + for (ci = c->cibase; ci <= c->ci; ci++) { + if (ci->proc) { + mrb_gc_mark(mrb, (struct RBasic*)ci->proc); + } + if (ci->u.target_class) { + mrb_gc_mark(mrb, (struct RBasic*)ci->u.target_class); + } + } + } + + /* Mark fiber */ + mrb_gc_mark(mrb, (struct RBasic*)c->fib); + + /* Mark task-specific values */ + mrb_gc_mark_value(mrb, t->self); + mrb_gc_mark_value(mrb, t->result); + mrb_gc_mark_value(mrb, t->name); + mrb_gc_mark_value(mrb, t->proc); + + t = t->next; + } + } +} + /* * Queue operations */ @@ -190,6 +258,7 @@ task_init_context(mrb_state *mrb, mrb_task *t, const struct RProc *proc) mrb_vm_ci_target_class_set(ci, MRB_PROC_TARGET_CLASS(proc)); mrb_vm_ci_proc_set(ci, proc); ci->stack = c->stbase; + ci->pc = proc->body.irep->iseq; /* Initialize PC to start of bytecode */ ci[1] = ci[0]; c->ci++; /* Push dummy callinfo */ @@ -275,6 +344,19 @@ mrb_tasks_run(mrb_state *mrb) break; } + /* Safety check - don't execute terminated tasks */ + if (t->status == MRB_TASKSTATUS_DORMANT || t->c.status == MRB_FIBER_TERMINATED) { + /* Task is terminated but still in queue - remove it */ + mrb_task_disable_irq(); + q_delete_task(mrb, t); + if (t->status != MRB_TASKSTATUS_DORMANT) { + t->status = MRB_TASKSTATUS_DORMANT; + q_insert_task(mrb, t); + } + mrb_task_enable_irq(); + continue; + } + /* Set task as running */ t->status = MRB_TASKSTATUS_RUNNING; t->timeslice = MRB_TIMESLICE_TICK_COUNT; @@ -833,12 +915,16 @@ mrb_task_s_new(mrb_state *mrb, mrb_value self) t->status = MRB_TASKSTATUS_READY; t->reason = MRB_TASKREASON_NONE; t->name = name_val; + t->proc = blk; /* Store proc to keep it from being GC'd */ /* Create Ruby object to hold task */ task_obj = mrb_obj_value(mrb_data_object_alloc(mrb, mrb_class_get(mrb, "Task"), t, &mrb_task_type)); t->self = task_obj; + /* Register with GC to protect task object from collection */ + mrb_gc_register(mrb, task_obj); + /* Initialize task context */ task_init_context(mrb, t, proc); @@ -861,13 +947,14 @@ mrb_task_s_new(mrb_state *mrb, mrb_value self) static mrb_value mrb_task_s_current(mrb_state *mrb, mrb_value self) { - mrb_task *t = q_ready_; - - if (t && t->status == MRB_TASKSTATUS_RUNNING) { - return t->self; + /* Check if we're in root context (not in a task) */ + if (mrb->c == mrb->root_c) { + return mrb_nil_value(); } - return mrb_nil_value(); + /* Use pointer arithmetic to get task from context - O(1) */ + mrb_task *t = MRB2TASK(mrb); + return t->self; } static mrb_value @@ -890,10 +977,9 @@ mrb_task_s_list(mrb_state *mrb, mrb_value self) static mrb_value mrb_task_s_pass(mrb_state *mrb, mrb_value self) { - /* Yield to other tasks by triggering a context switch */ - mrb_task *t = q_ready_; - - if (t && t->status == MRB_TASKSTATUS_RUNNING) { + /* Only yield if we're in a task context */ + if (mrb->c != mrb->root_c) { + /* Trigger context switch to yield to other tasks */ switching_ = TRUE; } @@ -1171,11 +1257,11 @@ mrb_task_join(mrb_state *mrb, mrb_value self) mrb_raise(mrb, E_ARGUMENT_ERROR, "invalid task"); } - /* Get current task */ - current = q_ready_; - if (!current || current->status != MRB_TASKSTATUS_RUNNING) { + /* Get current task using pointer arithmetic */ + if (mrb->c == mrb->root_c) { mrb_raise(mrb, E_RUNTIME_ERROR, "join can only be called from running task"); } + current = MRB2TASK(mrb); /* Can't join self */ if (t == current) { diff --git a/src/gc.c b/src/gc.c index f51d0bc1e..286b32c3e 100644 --- a/src/gc.c +++ b/src/gc.c @@ -28,6 +28,11 @@ #include #endif +#ifdef MRB_USE_TASK_SCHEDULER +/* Weak stub for mrb_task_mark_all - actual implementation in task.c */ +void __attribute__((weak)) mrb_task_mark_all(mrb_state *mrb) { (void)mrb; } +#endif + /* = Tri-color Incremental Garbage Collection @@ -956,6 +961,11 @@ root_scan_phase(mrb_state *mrb, mrb_gc *gc) if (mrb->root_c != mrb->c) { mark_context(mrb, mrb->root_c); } + +#ifdef MRB_USE_TASK_SCHEDULER + /* mark tasks - calls into task.c to mark all task queues */ + mrb_task_mark_all(mrb); +#endif } static void