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: <mailto:ruby-core-request / ruby-lang.org?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>