[ruby-core:45950] Re: [ruby-trunk - Feature #3620] Add Queue, SIzedQueue and ConditionVariable implementations in C in addition to ruby ones

Hi,

(2012/06/26 16:18), funny_falcon (Yura Sokolov) wrote:
> It seems that there is no need for mutex in a native queue implementation (considering we have GVL),
> and so that queue_synchronized wrapper:
> rb_mutex_sleep could be replaces with rb_thread_sleep_forever without semantic change.
> 
> Without mutex, native queue becomes 2 times faster.
> Modified patch is attached.

I found a bug.

### begin sample code
q = Thread::Queue.new
#q = Queue.new
th1 = Thread.new{
  begin
    p [:th1, q.pop]
  rescue RuntimeError => e
    sleep
    p e
  end
}
th2 = Thread.new{
  sleep 0.1
  p [:th2, q.pop]
}
p [th1, th2]
sleep 0.5
th1.raise "async interrupt!"
sleep 0.5
q << :s
# th1.join
p [th1, th2]
th2.join            # BLOCK forever!
### end sample code

When th1 escapes from blocking by "pop" by exception, then waiting list
of Queue should be maintained (remove th1 from waiting list).


Other comment:

* "extthread" is good name?

* variable name

static void
wakeup_all_threads(VALUE list)
{
    VALUE thread, list0 = list;
    long i;

    list = rb_ary_subseq(list, 0, LONG_MAX);
    rb_ary_clear(list0);
    for (i = 0; i < RARRAY_LEN(list); ++i) {
	thread = RARRAY_PTR(list)[i];
	rb_thread_wakeup_alive(thread);
    }
    RB_GC_GUARD(list);
}

I prefer:

static void
wakeup_all_threads(VALUE list0)
{
    VALUE thread, list = rb_ary_subseq(list0, 0, LONG_MAX);
    long i;

    rb_ary_clear(list0);
    for (i = 0; i < RARRAY_LEN(list); ++i) {
	thread = RARRAY_PTR(list)[i];
	rb_thread_wakeup_alive(thread);
    }
    RB_GC_GUARD(list);
}

But I prefer more:

static void
wakeup_all_threads(VALUE list)
{
    VALUE thread;
    long i;

    for (i = 0; i < RARRAY_LEN(list); ++i) {
	thread = RARRAY_PTR(list)[i];
	rb_thread_wakeup_alive(thread);
    }
    rb_ary_clear(list);
}

Any reason to dup array before iteration?

* T_DATA -> T_STRUCT or T_OBJECT

In this case, you don't need to use T_DATA.  You can only use T_STRUCT
or T_OBJECT (with hidden attr).  Maybe it will be simple.


Thanks,
Koichi


> ----------------------------------------
> Feature #3620: Add Queue, SIzedQueue and ConditionVariable implementations in C in addition to ruby ones
> https://bugs.ruby-lang.org/issues/3620#change-27471

-- 
// SASADA Koichi at atdot dot net