ruby-changes:53406
From: ko1 <ko1@a...>
Date: Thu, 8 Nov 2018 14:01:29 +0900 (JST)
Subject: [ruby-changes:53406] ko1:r65622 (trunk): separate Thread type (func or proc) explicitly.
ko1 2018-11-08 14:01:23 +0900 (Thu, 08 Nov 2018) New Revision: 65622 https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=65622 Log: separate Thread type (func or proc) explicitly. * vm_core.h (rb_thread_struct): introduce new fields `invoke_type` and `invoke_arg`. There are two types threads: invoking proc (normal Ruby thread created by `Thread.new do ... end`) and invoking func, created by C-API. `invoke_type` shows the types. * thread.c (thread_do_start): copy `invoke_arg.proc.args` contents from Array to ALLOCA stack memory if args length is enough small (<8). We don't need to keep Array and don't need to cancel using transient heap. * vm.c (thread_mark): For func invoking threads, they can pass (void *) parameter (rb_thread_t::invoke_arg::func::arg). However, a rubyspec test (thread_spec.c) passes an Array object and it expect to mark it. Clealy it is out of scope (misuse of `rb_thread_create` C-API). However, I'm not sure someone else has such kind of misunderstanding. So now we mark conservatively this (void *) arg with rb_gc_mark_maybe. This misuse is found by this error log. http://ci.rvm.jp/results/trunk-theap-asserts@silicon-docker/1448164 Modified files: trunk/thread.c trunk/thread_pthread.c trunk/vm.c trunk/vm_core.h Index: thread_pthread.c =================================================================== --- thread_pthread.c (revision 65621) +++ thread_pthread.c (revision 65622) @@ -1613,36 +1613,36 @@ setup_communication_pipe_internal(int pi https://github.com/ruby/ruby/blob/trunk/thread_pthread.c#L1613 # define SET_CURRENT_THREAD_NAME(name) prctl(PR_SET_NAME, name) #endif +static VALUE threadptr_invoke_proc_location(rb_thread_t *th); + static void native_set_thread_name(rb_thread_t *th) { #ifdef SET_CURRENT_THREAD_NAME - if (!th->first_func && th->first_proc) { - VALUE loc; - if (!NIL_P(loc = th->name)) { - SET_CURRENT_THREAD_NAME(RSTRING_PTR(loc)); - } - else if (!NIL_P(loc = rb_proc_location(th->first_proc))) { - char *name, *p; - char buf[16]; - size_t len; - int n; + VALUE loc; + if (!NIL_P(loc = th->name)) { + SET_CURRENT_THREAD_NAME(RSTRING_PTR(loc)); + } + else if ((loc = threadptr_invoke_proc_location(th)) != Qnil) { + char *name, *p; + char buf[16]; + size_t len; + int n; - name = RSTRING_PTR(RARRAY_AREF(loc, 0)); - p = strrchr(name, '/'); /* show only the basename of the path. */ - if (p && p[1]) - name = p + 1; + name = RSTRING_PTR(RARRAY_AREF(loc, 0)); + p = strrchr(name, '/'); /* show only the basename of the path. */ + if (p && p[1]) + name = p + 1; - n = snprintf(buf, sizeof(buf), "%s:%d", name, NUM2INT(RARRAY_AREF(loc, 1))); - rb_gc_force_recycle(loc); /* acts as a GC guard, too */ + n = snprintf(buf, sizeof(buf), "%s:%d", name, NUM2INT(RARRAY_AREF(loc, 1))); + rb_gc_force_recycle(loc); /* acts as a GC guard, too */ - len = (size_t)n; - if (len >= sizeof(buf)) { - buf[sizeof(buf)-2] = '*'; - buf[sizeof(buf)-1] = '\0'; - } - SET_CURRENT_THREAD_NAME(buf); - } + len = (size_t)n; + if (len >= sizeof(buf)) { + buf[sizeof(buf)-2] = '*'; + buf[sizeof(buf)-1] = '\0'; + } + SET_CURRENT_THREAD_NAME(buf); } #endif } Index: vm.c =================================================================== --- vm.c (revision 65621) +++ vm.c (revision 65622) @@ -2435,8 +2435,17 @@ thread_mark(void *ptr) https://github.com/ruby/ruby/blob/trunk/vm.c#L2435 rb_fiber_mark_self(th->ec->fiber_ptr); /* mark ruby objects */ - RUBY_MARK_UNLESS_NULL(th->first_proc); - if (th->first_proc) RUBY_MARK_UNLESS_NULL(th->first_args); + switch (th->invoke_type) { + case thread_invoke_type_proc: + RUBY_MARK_UNLESS_NULL(th->invoke_arg.proc.proc); + RUBY_MARK_UNLESS_NULL(th->invoke_arg.proc.args); + break; + case thread_invoke_type_func: + rb_gc_mark_maybe((VALUE)th->invoke_arg.func.arg); + break; + default: + break; + } RUBY_MARK_UNLESS_NULL(th->thgroup); RUBY_MARK_UNLESS_NULL(th->value); Index: vm_core.h =================================================================== --- vm_core.h (revision 65621) +++ vm_core.h (revision 65622) @@ -936,9 +936,22 @@ typedef struct rb_thread_struct { https://github.com/ruby/ruby/blob/trunk/vm_core.h#L936 rb_thread_list_t *join_list; - VALUE first_proc; - VALUE first_args; - VALUE (*first_func)(ANYARGS); + union { + struct { + VALUE proc; + VALUE args; + } proc; + struct { + VALUE (*func)(ANYARGS); + void *arg; + } func; + } invoke_arg; + + enum { + thread_invoke_type_none = 0, + thread_invoke_type_proc, + thread_invoke_type_func, + } invoke_type; /* statistics data for profiler */ VALUE stat_insn_usage; Index: thread.c =================================================================== --- thread.c (revision 65621) +++ thread.c (revision 65622) @@ -654,23 +654,42 @@ rb_vm_proc_local_ep(VALUE proc) https://github.com/ruby/ruby/blob/trunk/thread.c#L654 } static void -thread_do_start(rb_thread_t *th, VALUE args) +thread_do_start(rb_thread_t *th) { native_set_thread_name(th); - if (!th->first_func) { + + if (th->invoke_type == thread_invoke_type_proc) { + VALUE args = th->invoke_arg.proc.args; + long args_len = RARRAY_LEN(args); + const VALUE *args_ptr; + VALUE procval = th->invoke_arg.proc.proc; rb_proc_t *proc; - GetProcPtr(th->first_proc, proc); - th->ec->errinfo = Qnil; - th->ec->root_lep = rb_vm_proc_local_ep(th->first_proc); + GetProcPtr(procval, proc); + + th->ec->errinfo = Qnil; + th->ec->root_lep = rb_vm_proc_local_ep(procval); th->ec->root_svar = Qfalse; - EXEC_EVENT_HOOK(th->ec, RUBY_EVENT_THREAD_BEGIN, th->self, 0, 0, 0, Qundef); - th->value = rb_vm_invoke_proc(th->ec, proc, - (int)RARRAY_LEN(args), RARRAY_CONST_PTR(args), - VM_BLOCK_HANDLER_NONE); - EXEC_EVENT_HOOK(th->ec, RUBY_EVENT_THREAD_END, th->self, 0, 0, 0, Qundef); + + EXEC_EVENT_HOOK(th->ec, RUBY_EVENT_THREAD_BEGIN, th->self, 0, 0, 0, Qundef); + + if (args_len < 8) { + /* free proc.args if the length is enough small */ + args_ptr = ALLOCA_N(VALUE, args_len); + MEMCPY((VALUE *)args_ptr, RARRAY_CONST_PTR_TRANSIENT(args), VALUE, args_len); + th->invoke_arg.proc.args = Qnil; + } + else { + args_ptr = RARRAY_CONST_PTR(args); + } + + th->value = rb_vm_invoke_proc(th->ec, proc, + (int)args_len, args_ptr, + VM_BLOCK_HANDLER_NONE); + + EXEC_EVENT_HOOK(th->ec, RUBY_EVENT_THREAD_END, th->self, 0, 0, 0, Qundef); } else { - th->value = (*th->first_func)((void *)args); + th->value = (*th->invoke_arg.func.func)(th->invoke_arg.func.arg); } } @@ -680,7 +699,6 @@ static int https://github.com/ruby/ruby/blob/trunk/thread.c#L699 thread_start_func_2(rb_thread_t *th, VALUE *stack_start, VALUE *register_stack_start) { enum ruby_tag_type state; - VALUE args = th->first_args; rb_thread_list_t *join_list; rb_thread_t *main_th; VALUE errinfo = Qnil; @@ -703,7 +721,7 @@ thread_start_func_2(rb_thread_t *th, VAL https://github.com/ruby/ruby/blob/trunk/thread.c#L721 EC_PUSH_TAG(th->ec); if ((state = EC_EXEC_TAG()) == TAG_NONE) { - SAVE_ROOT_JMPBUF(th, thread_do_start(th, args)); + SAVE_ROOT_JMPBUF(th, thread_do_start(th)); } else { errinfo = th->ec->errinfo; @@ -793,10 +811,16 @@ thread_create_core(VALUE thval, VALUE ar https://github.com/ruby/ruby/blob/trunk/thread.c#L811 "can't start a new thread (frozen ThreadGroup)"); } - /* setup thread environment */ - th->first_func = fn; - th->first_proc = fn ? Qfalse : rb_block_proc(); - th->first_args = args; /* GC: shouldn't put before above line */ + if (fn) { + th->invoke_type = thread_invoke_type_func; + th->invoke_arg.func.func = fn; + th->invoke_arg.func.arg = (void *)args; + } + else { + th->invoke_type = thread_invoke_type_proc; + th->invoke_arg.proc.proc = rb_block_proc(); + th->invoke_arg.proc.args = args; + } th->priority = current_th->priority; th->thgroup = current_th->thgroup; @@ -818,7 +842,7 @@ thread_create_core(VALUE thval, VALUE ar https://github.com/ruby/ruby/blob/trunk/thread.c#L842 return thval; } -#define threadptr_initialized(th) ((th)->first_args != 0) +#define threadptr_initialized(th) ((th)->invoke_type != thread_invoke_type_none) /* * call-seq: @@ -874,6 +898,17 @@ thread_start(VALUE klass, VALUE args) https://github.com/ruby/ruby/blob/trunk/thread.c#L898 return thread_create_core(rb_thread_alloc(klass), args, 0); } +static VALUE +threadptr_invoke_proc_location(rb_thread_t *th) +{ + if (th->invoke_type == thread_invoke_type_proc) { + return rb_proc_location(th->invoke_arg.proc.proc); + } + else { + return Qnil; + } +} + /* :nodoc: */ static VALUE thread_initialize(VALUE thread, VALUE args) @@ -881,19 +916,21 @@ thread_initialize(VALUE thread, VALUE ar https://github.com/ruby/ruby/blob/trunk/thread.c#L916 rb_thread_t *th = rb_thread_ptr(thread); if (!rb_block_given_p()) { - rb_raise(rb_eThreadError, "must be called with a block"); + rb_raise(rb_eThreadError, "must be called with a block"); } - else if (th->first_args) { - VALUE proc = th->first_proc, loc; - if (!proc || !RTEST(loc = rb_proc_location(proc))) { - rb_raise(rb_eThreadError, "already initialized thread"); - } - rb_raise(rb_eThreadError, - "already initialized thread - %"PRIsVALUE":%"PRIsVALUE, - RARRAY_AREF(loc, 0), RARRAY_AREF(loc, 1)); + else if (th->invoke_type != thread_invoke_type_none) { + VALUE loc = threadptr_invoke_proc_location(th); + if (!NIL_P(loc)) { + rb_raise(rb_eThreadError, + "already initialized thread - %"PRIsVALUE":%"PRIsVALUE, + RARRAY_AREF(loc, 0), RARRAY_AREF(loc, 1)); + } + else { + rb_raise(rb_eThreadError, "already initialized thread"); + } } else { - return thread_create_core(thread, args, 0); + return thread_create_core(thread, args, NULL); } } @@ -3081,20 +3118,17 @@ rb_thread_to_s(VALUE thread) https://github.com/ruby/ruby/blob/trunk/thread.c#L3118 VALUE cname = rb_class_path(rb_obj_class(thread)); rb_thread_t *target_th = rb_thread_ptr(thread); const char *status; - VALUE str; + VALUE str, loc; status = thread_status_name(target_th, TRUE); str = rb_sprintf("#<%"PRIsVALUE":%p", cname, (void *)thread); if (!NIL_P(target_th->name)) { - rb_str_catf(str, "@%"PRIsVALUE, target_th->name); + rb_str_catf(str, "@%"PRIsVALUE, target_th->name); } - if (!target_th->first_func && target_th->first_proc) { - VALUE loc = rb_proc_location(target_th->first_proc); - if (!NIL_P(loc)) { - rb_str_catf(str, "@%"PRIsVALUE":%"PRIsVALUE, - RARRAY_AREF(loc, 0), RARRAY_AREF(loc, 1)); - rb_gc_force_recycle(loc); - } + if ((loc = threadptr_invoke_proc_location(target_th)) != Qnil) { + rb_str_catf(str, "@%"PRIsVALUE":%"PRIsVALUE, + RARRAY_AREF(loc, 0), RARRAY_AREF(loc, 1)); + rb_gc_force_recycle(loc); } rb_str_catf(str, " %s>", status); OBJ_INFECT(str, thread); -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/