Re: [PATCH 1/2] add `#kgio_writev` and `#kgio_trywritev`

[Eric Wong <normalperson@yhbt.net>]

Yura Sokolov <funny.falcon@gmail.com> wrote:
> 29.05.2012 23:11, Eric Wong написал:
> > Sokolov Yura 'funny-falcon<funny.falcon@gmail.com>  wrote:
> >> Since both methods dups array and strings in it, `#kgio_writev` semantic
> >> a bit different from `#kgio_write`: it does not react on changes to
> >> array/strings that made in other thread. But I think, this way is more
> >> correct.
> >
> > Perhaps we should use rb_str_locktmp()?
> 
> I think, it is good idea :) And use it at `#kgio_write` as well.
> 
> I want to redo patch: #kgio_trywritev needs not dup-ing or freeze-ing/lock-ing,
> and this method is primary for me.
> May be, array should be slice-ed when it is not fully written? It will do
> the good job for #kgio_write too.
> 
> (I ommit some formatting comments, will fix them)

OK, thank you for doing this :>

> >> +				n -= RSTRING_LEN(str);
> >> +				rb_ary_shift(a->buf);
> >> +				a->vec++;
> >
> > Probably better to store the array offset instead of shifting
> > Array#shift is O(n).
> Well, in Ruby1.9 it is O(1) if not intermixed with other modifying calls.
> I'll use rb_ary_subseq, it is also O(1) in Ruby1.9.

rb_ary_subseq can still copy or (at the very least) alloc a new object,
though.  Just storing the offset as a long and calling rb_ary_entry()
will be simplest, I think.

Some of the optimizations in MRI array.c make it hard to follow :<

> >> +retry:
> >> +	iov_cnt = RARRAY_LEN(a.buf);
> >> +	if (iov_cnt>  iov_max) iov_cnt = iov_max;
> >> +	n = (long)writev(a.fd, a.vec, iov_cnt);
> >> +	if (writev_check(&a, n, "writev", io_wait) != 0)
> >> +		goto retry;
> >> +	if (TYPE(a.buf) != T_SYMBOL)
> >> +		kgio_autopush_write(io);
> >> +	return a.buf;
> >
> > (I'm not fully awake): You do retry when truncating to iov_max, right?
> 
> Retry is done cause writev_check returns -1

OK, thanks for confirming that.

> I'll try to redo patch today or tomorrow.

Thank you again!