ruby-changes:53061
From: k0kubun <ko1@a...>
Date: Sun, 21 Oct 2018 22:21:35 +0900 (JST)
Subject: [ruby-changes:53061] k0kubun:r65275 (trunk): mjit.c: copy inline cache values to MJIT worker
k0kubun 2018-10-21 22:21:28 +0900 (Sun, 21 Oct 2018) New Revision: 65275 https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=65275 Log: mjit.c: copy inline cache values to MJIT worker on VM_CHECK_INTS. Letting MJIT worker directly see inline cache which may be being updated could result in inconsistent IC index and serial. mjit_worker.c: request the copy job after dequeue, and receive the result synchronously. tool/ruby_vm/views/_mjit_compile_ivar.erb: use the copied IC mjit_compile.c: change the interface to pass is_entries mjit.h: ditto === Optcarrot Benchmark === Thankfully this didn't have major performance regression. $ benchmark-driver benchmark.yml --rbenv 'before::before --disable-gems --jit;after::after --disable-gems --jit' -v --repeat-count 24 before: ruby 2.6.0dev (2018-10-21 trunk 65263) +JIT [x86_64-linux] after: ruby 2.6.0dev (2018-10-21 trunk 65263) +JIT [x86_64-linux] last_commit=mjit.c: copy inline cache values to MJIT worker Calculating ------------------------------------- before after Optcarrot Lan_Master.nes 85.421 85.454 fps Comparison: Optcarrot Lan_Master.nes after: 85.5 fps before: 85.4 fps - 1.00x slower Modified files: trunk/mjit.c trunk/mjit.h trunk/mjit_compile.c trunk/mjit_worker.c trunk/tool/ruby_vm/views/_mjit_compile_ivar.erb Index: tool/ruby_vm/views/_mjit_compile_ivar.erb =================================================================== --- tool/ruby_vm/views/_mjit_compile_ivar.erb (revision 65274) +++ tool/ruby_vm/views/_mjit_compile_ivar.erb (revision 65275) @@ -13,17 +13,19 @@ https://github.com/ruby/ruby/blob/trunk/tool/ruby_vm/views/_mjit_compile_ivar.erb#L13 % insn.opes.each_with_index do |ope, i| MAYBE_UNUSED(<%= ope.fetch(:decl) %>) = (<%= ope.fetch(:type) %>)operands[<%= i %>]; % end +% # compiler: Use copied IC to avoid race condition + IC ic_copy = &(status->is_entries + ((union iseq_inline_storage_entry *)ic - body->is_entries))->cache; % -% # compiler: Consider cfp->self as T_OBJECT if ic->ic_serial is set - if (ic->ic_serial) { +% # compiler: Consider cfp->self as T_OBJECT if ic_copy->ic_serial is set + if (ic_copy->ic_serial) { % # JIT: optimize away motion of sp and pc. This path does not call rb_warning() and so it's always leaf and not `handles_sp`. % # <%= render 'mjit_compile_pc_and_sp', locals: { insn: insn } -%> % % # JIT: prepare vm_getivar's arguments and variables fprintf(f, "{\n"); fprintf(f, " VALUE obj = GET_SELF();\n"); - fprintf(f, " const rb_serial_t ic_serial = (rb_serial_t)%"PRI_SERIALT_PREFIX"u;\n", ic->ic_serial); - fprintf(f, " const st_index_t index = %"PRIuSIZE";\n", ic->ic_value.index); + fprintf(f, " const rb_serial_t ic_serial = (rb_serial_t)%"PRI_SERIALT_PREFIX"u;\n", ic_copy->ic_serial); + fprintf(f, " const st_index_t index = %"PRIuSIZE";\n", ic_copy->ic_value.index); % if insn.name == 'setinstancevariable' fprintf(f, " VALUE val = stack[%d];\n", b->stack_size - 1); % end Index: mjit_worker.c =================================================================== --- mjit_worker.c (revision 65274) +++ mjit_worker.c (revision 65275) @@ -76,6 +76,7 @@ https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L76 #include "mjit.h" #include "gc.h" #include "ruby_assert.h" +#include "ruby/debug.h" #include "ruby/thread.h" #ifdef _WIN32 @@ -1028,7 +1029,7 @@ compile_prelude(FILE *f) https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L1029 /* Compile ISeq in UNIT and return function pointer of JIT-ed code. It may return NOT_COMPILED_JIT_ISEQ_FUNC if something went wrong. */ static mjit_func_t -convert_unit_to_func(struct rb_mjit_unit *unit) +convert_unit_to_func(struct rb_mjit_unit *unit, union iseq_inline_storage_entry *is_entries) { char c_file_buff[MAXPATHLEN], *c_file = c_file_buff, *so_file, funcname[35]; /* TODO: reconsider `35` */ int success; @@ -1096,7 +1097,7 @@ convert_unit_to_func(struct rb_mjit_unit https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L1097 verbose(2, "start compilation: %s@%s:%d -> %s", label, path, lineno, c_file); fprintf(f, "/* %s@%s:%d */\n\n", label, path, lineno); } - success = mjit_compile(f, unit->iseq->body, funcname); + success = mjit_compile(f, unit->iseq->body, funcname, is_entries); /* release blocking mjit_gc_start_hook */ CRITICAL_SECTION_START(3, "after mjit_compile to wakeup client for GC"); @@ -1160,6 +1161,14 @@ convert_unit_to_func(struct rb_mjit_unit https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L1161 return (mjit_func_t)func; } +struct mjit_copy_job { + const struct rb_iseq_constant_body *body; + union iseq_inline_storage_entry *is_entries; + int finish_p; +}; + +static void mjit_copy_job_handler(void *data); + /* The function implementing a worker. It is executed in a separate thread by rb_thread_create_mjit_thread. It compiles precompiled header and then compiles requested ISeqs. */ @@ -1195,7 +1204,27 @@ mjit_worker(void) https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L1204 CRITICAL_SECTION_FINISH(3, "in worker dequeue"); if (node) { - mjit_func_t func = convert_unit_to_func(node->unit); + mjit_func_t func; + struct mjit_copy_job job; + + /* Copy ISeq's inline caches from main thread. */ + job.is_entries = NULL; + job.body = node->unit->iseq->body; + if (job.body->is_size > 0) { + job.is_entries = malloc(sizeof(union iseq_inline_storage_entry) * job.body->is_size); + job.finish_p = FALSE; + + rb_postponed_job_register(0, mjit_copy_job_handler, (void *)&job); + CRITICAL_SECTION_START(3, "in MJIT copy job wait"); + while (!job.finish_p) { + rb_native_cond_wait(&mjit_worker_wakeup, &mjit_engine_mutex); + verbose(3, "Getting wakeup from client"); + } + CRITICAL_SECTION_FINISH(3, "in MJIT copy job wait"); + } + + /* JIT compile */ + func = convert_unit_to_func(node->unit, job.is_entries); CRITICAL_SECTION_START(3, "in jit func replace"); if (node->unit->iseq) { /* Check whether GCed or not */ @@ -1212,6 +1241,9 @@ mjit_worker(void) https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L1241 compact_all_jit_code(); } #endif + if (job.is_entries != NULL) { + free(job.is_entries); + } } } Index: mjit.c =================================================================== --- mjit.c (revision 65274) +++ mjit.c (revision 65275) @@ -20,6 +20,19 @@ https://github.com/ruby/ruby/blob/trunk/mjit.c#L20 #include "constant.h" #include "id_table.h" +/* Copy ISeq's states so that race condition does not happen on compilation. */ +static void +mjit_copy_job_handler(void *data) +{ + struct mjit_copy_job *job = (struct mjit_copy_job *)data; + memcpy(job->is_entries, job->body->is_entries, sizeof(union iseq_inline_storage_entry) * job->body->is_size); + + CRITICAL_SECTION_START(3, "in MJIT copy job wait"); + job->finish_p = TRUE; + rb_native_cond_broadcast(&mjit_worker_wakeup); + CRITICAL_SECTION_FINISH(3, "in MJIT copy job wait"); +} + extern int rb_thread_create_mjit_thread(void (*worker_func)(void)); /* Return an unique file name in /tmp with PREFIX and SUFFIX and Index: mjit.h =================================================================== --- mjit.h (revision 65274) +++ mjit.h (revision 65275) @@ -64,7 +64,7 @@ extern void mjit_add_iseq_to_process(con https://github.com/ruby/ruby/blob/trunk/mjit.h#L64 extern VALUE mjit_wait_call(rb_execution_context_t *ec, struct rb_iseq_constant_body *body); RUBY_SYMBOL_EXPORT_END -extern int mjit_compile(FILE *f, const struct rb_iseq_constant_body *body, const char *funcname); +extern int mjit_compile(FILE *f, const struct rb_iseq_constant_body *body, const char *funcname, union iseq_inline_storage_entry *is_entries); extern void mjit_init(struct mjit_options *opts); extern void mjit_finish(void); extern void mjit_gc_start_hook(void); Index: mjit_compile.c =================================================================== --- mjit_compile.c (revision 65274) +++ mjit_compile.c (revision 65275) @@ -34,6 +34,8 @@ struct compile_status { https://github.com/ruby/ruby/blob/trunk/mjit_compile.c#L34 /* If TRUE, JIT-ed code will use local variables to store pushed values instead of using VM's stack and moving stack pointer. */ int local_stack_p; + /* Safely-accessible is_entries copied from main thread. */ + union iseq_inline_storage_entry *is_entries; }; /* Storage to keep data which is consistent in each conditional branch. @@ -195,7 +197,7 @@ compile_cancel_handler(FILE *f, const st https://github.com/ruby/ruby/blob/trunk/mjit_compile.c#L197 /* Compile ISeq to C code in F. It returns 1 if it succeeds to compile. */ int -mjit_compile(FILE *f, const struct rb_iseq_constant_body *body, const char *funcname) +mjit_compile(FILE *f, const struct rb_iseq_constant_body *body, const char *funcname, union iseq_inline_storage_entry *is_entries) { struct compile_status status; status.success = TRUE; @@ -204,6 +206,7 @@ mjit_compile(FILE *f, const struct rb_is https://github.com/ruby/ruby/blob/trunk/mjit_compile.c#L206 if (status.stack_size_for_pos == NULL) return FALSE; memset(status.stack_size_for_pos, NOT_COMPILED_STACK_SIZE, sizeof(int) * body->iseq_size); + status.is_entries = is_entries; /* For performance, we verify stack size only on compilation time (mjit_compile.inc.erb) without --jit-debug */ if (!mjit_opts.debug) { -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/