From: Eric Wong Date: 2018-08-15T00:21:17+00:00 Subject: [ruby-core:88488] Re: [Ruby trunk Misc#14937] [PATCH] thread_pthread: lazy-spawn timer-thread only on contention ko1@atdot.net wrote: > Let me clear your commits. > Maybe "Description" of this ticket is obsolete. I guess, I will add doc or comments to thread_pthread.c > They are my understandings (fix me if they are wrong): > > * Your idea is using POSIX timer if possible. Yes, but scope of "timer" here is reduced. Old timer-thread did several things: 1) set timer-interrupt (100ms timeslice) 2) handle ubf list 3) check for signals New GVL can handle all of these tasks. However, application without GVL contention need UBF_TIMER_* to deal with 2) and 3) reliably (unless [Bug #14968] to make all pipes/sockets non-blocking is accepted). > ``` > #define UBF_TIMER_NONE 0 > #define UBF_TIMER_POSIX 1 > #define UBF_TIMER_PTHREAD 2 > ``` > > `UBF_TIMER_POSIX` uses POSIX timer. `UBF_TIMER_PTHREAD` is traditional timer thread approach. Not exactly. UBF_TIMER_* does not handle 1) (timer-interrupt) > BTW > > ``` > * UBF_TIMER is to close TOCTTOU signal race on programs > * without GVL contention blocking read/write to sockets. > ``` > > I can't understand these lines. > What is related between GVL contention and read/write sockets? > Single thread application do not kick `gvl_acquire_common` and `timer_thread_function()` is not kicked, right? Right. > * GVL acquire loop invoke `timer_thread_function()` > > ... sorry I'm giving up to understand them correctly. I think the confusing thing is UBF_TIMER is NOT for timer-interrupt. timer_thread_function is only for timer-interrupt, now (not signals or ubf_list). Also, timer_thread_pipe needs to be renamed, since it has nothing to do with timer, now. > Could you explain more for current trunk? > > After your commit, there are no timer threads? Correct (for platforms with POSIX timers, at least). > # Comments/Questions > > ## `gvl_acquire_common` > > > ` VM_ASSERT(th->unblock.func == 0 && "we reuse ubf_list for GVL waitq");` > > I strongly disagree that reusing. It is very confusing. I don't want to make rb_thread_struct any bigger. I can make it a union to clarify use. > > `rb_timer_(|dis)arm()` > > English question (sorry). What does it mean with word "arm"? "arm" as in enable the periodic timer, "disarm" stops the timer. "arm"/"disarm" is used in Linux timer_settime(2) manpage. Ditto for setitimer(2) manpage (setitimer was used in Ruby 1.8; but POSIX obsoletes it in favor of timer_settime). > > `native_cond_timeout()` > > This function is not good name because I misunderstand it is same as `native_cond_timedwait()`. > Maybe I had named it :p Yes, I just used an existing function :P > > ` list_add_tail(&vm->gvl.waitq, &nd->ubf_list);` > > Why add it? What`vm->gvl.waitq` manages? > To specify `designate_timer_thread()` Yes, basically we manage the GVL wait-queue ourselves, now, instead of relying on same linked-list in glibc. This is similar to my Mutex and autoload locking rewrite; as well as waitqueue implementation in Linux kernel. This allows us more fine-grained control and ordering of GVL waiters. > There are two condvars `gvlq` and `switch_cond`. Is it intentional? Yes, switch_cond is from old GVL by kosaki. It made Thread.pass much faster. I tried to get rid of switch_cond and switch_wait_cond, but could not match Thread.pass performance from old GVL. > Who remove it from the list? `list_top()` removes? (impl doesn't seem such deletion). The same thread which calls list_add_tail calls list_del_init in gvl_acquire_common. > > `ubf_wakeup_all_threads();` > > why wakeup all blocking threads? ubf_wakeup_all_threads always needs to be called periodically if there's threads in ubf_list. If GVL is contended, it is easy to do it from the thread which is already handling timer interrupt. > ## `rb_timer_create()` > > There is a guard `#if UBF_TIMER == UBF_TIMER_POSIX` but not for `rb_timer_pthread_create()`. > I misunderstand that `UBF_TIMER_POSIX` needs timer pthread. Right, I tried to avoid extra ifdef there since rb_timer_pthread_create is empty function for UBF_TIMER_POSIX anyways. I can add "if (UBF_TIMER == UBF_TIMER_PTHREAD)" conditional in C (not CPP) if it clarifies things. I prefer to avoid CPP ifdef guard when possible and efficient to do compiler checks in more platform-specific code. > > ## `rb_timer_` prefix > > making `timer_` will help to understand they are local static functions. > If you want to expose them outside thread_pthread.c, they should be more verbose name like `rb_vm_timer` and so on to recognize they are no `Timer` class in Ruby world (it is my opinion). OK, I will change to "ubf_timer_" prefix, instead. Bare "timer_" is confusing with POSIX functions, and I don't think they need to be exposed outside of thread_pthread.c Unsubscribe: