k0kubun	2019-03-18 02:12:47 +0900 (Mon, 18 Mar 2019)

  New Revision: 67286

  https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=67286

  Log:
    Drop rb_mjit_unit from mjit_copy_job
    
    and guard iseq from GC by marking iseq in mjit_copy_job.
    
    This is a refactoring for implementing inlining later and
    should not be fixing or introducing any bugs.

  Modified files:
    trunk/mjit.c
    trunk/mjit_worker.c
Index: mjit.c
===================================================================
--- mjit.c	(revision 67285)
+++ mjit.c	(revision 67286)
@@ -25,22 +25,21 @@ static void https://github.com/ruby/ruby/blob/trunk/mjit.c#L25
 mjit_copy_job_handler(void *data)
 {
     mjit_copy_job_t *job = data;
-    const struct rb_iseq_constant_body *body;
     if (stop_worker_p) { /* check if mutex is still alive, before calling CRITICAL_SECTION_START. */
         return;
     }
 
     CRITICAL_SECTION_START(3, "in mjit_copy_job_handler");
-    /* Make sure that this job is never executed when:
-       1. job is being modified
-       2. alloca memory inside job is expired
-       3. ISeq is GC-ed */
-    if (job->finish_p || job->unit->iseq == NULL) {
+    // Make sure that this job is never executed when:
+    //   1. job is being modified
+    //   2. alloca memory inside job is expired
+    // Note that job->iseq is guarded from GC by `mjit_mark`.
+    if (job->finish_p) {
         CRITICAL_SECTION_FINISH(3, "in mjit_copy_job_handler");
         return;
     }
 
-    body = job->unit->iseq->body;
+    const struct rb_iseq_constant_body *body = job->iseq->body;
     if (job->cc_entries) {
         memcpy(job->cc_entries, body->cc_entries, sizeof(struct rb_call_cache) * (body->ci_size + body->ci_kw_size));
     }
@@ -828,14 +827,23 @@ mjit_finish(bool close_handle_p) https://github.com/ruby/ruby/blob/trunk/mjit.c#L827
 void
 mjit_mark(void)
 {
-    struct rb_mjit_unit *unit = 0;
     if (!mjit_enabled)
         return;
     RUBY_MARK_ENTER("mjit");
+
+    CRITICAL_SECTION_START(4, "mjit_mark");
+    VALUE iseq = (VALUE)mjit_copy_job.iseq;
+    CRITICAL_SECTION_FINISH(4, "mjit_mark");
+
+    // Don't wrap critical section with this. This may trigger GC,
+    // and in that case mjit_gc_start_hook causes deadlock.
+    if (iseq) rb_gc_mark(iseq);
+
+    struct rb_mjit_unit *unit = NULL;
     CRITICAL_SECTION_START(4, "mjit_mark");
     list_for_each(&unit_queue.head, unit, unode) {
         if (unit->iseq) { /* ISeq is still not GCed */
-            VALUE iseq = (VALUE)unit->iseq;
+            iseq = (VALUE)unit->iseq;
             CRITICAL_SECTION_FINISH(4, "mjit_mark rb_gc_mark");
 
             /* Don't wrap critical section with this. This may trigger GC,
@@ -846,6 +854,7 @@ mjit_mark(void) https://github.com/ruby/ruby/blob/trunk/mjit.c#L854
         }
     }
     CRITICAL_SECTION_FINISH(4, "mjit_mark");
+
     RUBY_MARK_LEAVE("mjit");
 }
 
Index: mjit_worker.c
===================================================================
--- mjit_worker.c	(revision 67285)
+++ mjit_worker.c	(revision 67286)
@@ -1126,7 +1126,7 @@ convert_unit_to_func(struct rb_mjit_unit https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L1126
 }
 
 typedef struct {
-    struct rb_mjit_unit *unit;
+    const rb_iseq_t *iseq;
     struct rb_call_cache *cc_entries;
     union iseq_inline_storage_entry *is_entries;
     bool finish_p;
@@ -1134,7 +1134,7 @@ typedef struct { https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L1134
 
 /* Singleton MJIT copy job. This is made global since it needs to be durable even when MJIT worker thread is stopped.
    (ex: register job -> MJIT pause -> MJIT resume -> dispatch job. Actually this should be just cancelled by finish_p check) */
-static mjit_copy_job_t mjit_copy_job;
+static mjit_copy_job_t mjit_copy_job = { .iseq = NULL, .finish_p = true };
 
 static void mjit_copy_job_handler(void *data);
 
@@ -1204,14 +1204,12 @@ mjit_worker(void) https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L1204
             verbose(3, "Getting wakeup from client");
         }
         unit = get_from_list(&unit_queue);
+        if (unit) job->iseq = unit->iseq;
         job->finish_p = true; // disable dispatching this job in mjit_copy_job_handler while it's being modified
         CRITICAL_SECTION_FINISH(3, "in worker dequeue");
 
         if (unit) {
-            mjit_func_t func;
             const struct rb_iseq_constant_body *body = unit->iseq->body;
-
-            job->unit = unit;
             job->cc_entries = NULL;
             if (body->ci_size > 0 || body->ci_kw_size > 0)
                 job->cc_entries = alloca(sizeof(struct rb_call_cache) * (body->ci_size + body->ci_kw_size));
@@ -1226,8 +1224,8 @@ mjit_worker(void) https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L1224
                 }
             }
 
-            /* JIT compile */
-            func = convert_unit_to_func(unit, job->cc_entries, job->is_entries);
+            // JIT compile
+            mjit_func_t func = convert_unit_to_func(unit, job->cc_entries, job->is_entries);
 
             CRITICAL_SECTION_START(3, "in jit func replace");
             while (in_gc) { /* Make sure we're not GC-ing when touching ISeq */

--
ML: ruby-changes@q...
Info: http://www.atdot.net/~ko1/quickml/