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

[Eric Wong <normalperson@yhbt.net>]

Sokolov Yura 'funny-falcon <funny.falcon@gmail.com> wrote:

All the issues are pretty minor.  I can fix them if you don't
have time to fix them, but in case you have other revisions...

> +#ifndef HAVE_RB_ARY_SUBSEQ
> +static inline VALUE rb_ary_subseq(VALUE ary, long idx, long len)
> +{
> +	VALUE args[2] = {LONG2FIX(idx), LONG2FIX(len)};
> +	return rb_ary_aref(2, args, ary);
> +}

Lately, I've been favoring naming replacement functions differently
and using a macro (I need to fix this in tryopen.c, too :x) like this:

static inline VALUE my_ary_subseq(VALUE ary, long idx, long len)
{
	...
}
#define rb_ary_subseq(ary,idx,len) my_ary_subseq((ary),(idx),(len))

This makes it easier to debug (especially when I'm debugging
with optimizations/disabled).

> +struct io_args_v {
> +	VALUE io;
> +	VALUE buf;
> +	VALUE vec_buf;
> +	struct iovec *vec;
> +	int iov_cnt;

Might be better to make iov_cnt long since you're:

* assigning RARRAY_LEN to it
* comparing it to iov_max (a long)

And only cast iov+cnt to int when calling writev().

> +	int something_written;
> +	int fd;
> +};

> +static long trim_writev_buffer(struct io_args_v *a, long n)
> +{
> +	long i, ary_len = RARRAY_LEN(a->buf), str_len = 0;

Use one line for each statement if you're declaring + assigning:

	long i;
	long ary_len = RARRAY_LEN(a->buf);
	long str_len = 0;

> +	VALUE *elem = RARRAY_PTR(a->buf), str = 0;

No need to assign str (and if you did, I prefer NULL to zero unlike the
MRI team).  I think you can just make str local in each block it's used.

Needless assigning to zero can waste space on some compilers.  Not sure
if it's the case with modern GCC, but I remember it was wasteful in GCC
at some point in the last decade.

> +	for (i = 0; n && i < ary_len; i++, elem++) {
> +		str = *elem;
> +		str_len = RSTRING_LEN(str);
> +		n -= str_len;
> +		if (n < 0) break;
> +	}

Adding comments for my own benefit:

	/* all done */

> +	if (i == ary_len) {
> +		assert(n == 0 && "writev system call is broken");
> +		a->buf = Qnil;
> +		return 0;
> +	}

	/* partially done, remove fully-written buffers */

> +	if (i > 0) {
> +		a->buf = rb_ary_subseq(a->buf, i, ary_len - i);
> +	}

	if (i > 0)
		a->buf = rb_ary_subseq(a->buf, i, ary_len - i);

	/* setup+replace partially written buffer */

> +	if (n < 0) {
> +		str = rb_str_subseq(str, str_len + n, -n);
> +		rb_ary_store(a->buf, 0, str);
> +	}

> --- /dev/null
> +++ b/ext/kgio/writev_compat.h
> @@ -0,0 +1,57 @@
> +/*
> + * this header for supporting strange systems which missing writev
> + */
> +#if !defined(HAVE_WRITEV) && !defined(writev)
> +#define writev writev_supl
> +#define iovec  iovec_supl

I'd name the replacements and define macros the other way around.

> +#define WRITEV_MEMLIMIT (2*1024*1024)

I'm not sure if the mem limit is a good idea.  writev is meant to be
atomic (at least to regular files), and the strings are in memory,
anyways, I don't think having an extra copy will users hurt badly.
Don't need to worry about mmap()'ed regions in Ruby, either.

> +#define IOV_MAX 1024
> +
> +struct iovec {
> +	void  *iov_base;
> +	size_t iov_len;
> +};
> +
> +static ssize_t writev(int fd, const struct iovec *vec, int iov_cnt)
> +{
> +	int i;
> +	long result;

writev() result should be ssize_t for consistency.

> +	size_t total = 0;

No need to initialize to zero, since it's obviously assigned below.

> +	const struct iovec *curv = vec;
> +	char *buf, *cur;
> +
> +	if (iov_cnt == 0) return 0;
> +
> +	i = 1;
> +	total = curv->iov_len;
> +	if ( total < WRITEV_MEMLIMIT ) {
> +		for (curv++; i < iov_cnt; i++, curv++) {
> +			size_t next = total + curv->iov_len;
> +			if ( next > WRITEV_MEMLIMIT ) break;

minor: no space after "("  or before ")"

> +			total = next;
> +		}
> +	}
> +	iov_cnt = i;
> +
> +	if (iov_cnt > 1) {
> +		cur = buf = (char*)malloc(total);
> +		if (!buf) rb_memerror();

All Rubies with C API provide xmalloc(), use that instead, especially
since you're already using rb_memerror().

Also, never need to cast "void *" result to a different pointer type.

> +		curv = vec;
> +		for (i = 0; i < iov_cnt; i++, curv++) {
> +			memcpy(cur, curv->iov_base, curv->iov_len);
> +			cur += curv->iov_len;
> +		}
> +	} else {
> +		buf = vec->iov_base;
> +	}
> +
> +	result = (long)write(fd, buf, total);

ditto about result being ssize_t, no cast needed

> +	if (iov_cnt > 1) {
> +		free(buf);
> +	}

I suggest preserving errno from write(), free() can call brk/sbrk/munmap
that fail and set errno.

	if (iov_cnt > 1) {
		int save_errno = errno;
		free(buf);
		errno = save_errno;
	}


> +  def test_monster_writev_wait_writable
> +    @wr.instance_variable_set :@nr, 0
> +    def @wr.kgio_wait_writable
> +      @nr += 1
> +      IO.select(nil, [self])
> +    end
> +    buf = ["." * 1024] * 1024 * 10
> +    buf_size = buf.inject(0){|c, s| c + s.size}
> +    thr = Thread.new { @wr.kgio_writev(buf) }

I had trouble with this test on the last assertion.

Adding this here helped:

        Thread.pass until thr.stop?

(Using a dual core machine with plenty of memory for skbs).

> +    readed = @rd.read(buf_size)
> +    thr.join
> +    assert_nil thr.value
> +    assert_equal buf.join, readed
> +    assert @wr.instance_variable_get(:@nr) > 0
> +  end