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

[Eric Wong <normalperson@yhbt.net>]

Sokolov Yura 'funny-falcon <funny.falcon@gmail.com> wrote:
> +/* it seems that socket buffer is rarely exceed 1MB */

See other email about LFN.  On the other hand, 1M may be a good size
anyways to fit into the CPU cache.  Buffer size tuning is hard so
I usually avoid it :)

> +#define WRITEV_MEMLIMIT (1024*1024)
> +#define WRITEV_IMPL_THRESHOLD 512

I would like to see data/examples/benchmarks for how you arrived at the
WRITEV_IMPL_THRESHOLD.

This patch looks pretty good. More comments below.

> +static unsigned int iov_max = 1024; /* this could be overriden in init */
> +
> +struct io_args_v {
> +	VALUE io;
> +	VALUE buf;
> +	VALUE vec_buf;
> +	struct iovec *vec;
> +	unsigned long iov_cnt;
> +	size_t iov_len;

The "iov_len" name confused me, I think something with "total" is better.
Perhaps "total_len"?

> +static void prepare_writev(struct io_args_v *a, VALUE io, VALUE ary)
> +{
> +	long vec_cnt;
> +	static struct iovec stub = {NULL, 0};
> +	a->io = io;
> +	a->fd = my_fileno(io);
> +	a->something_written = 0;
> +
> +	/* rb_ary_subseq will not copy array unless it modified */
> +	a->buf = rb_ary_subseq(ary, 0, RARRAY_LEN(ary));
> +
> +	a->vec_buf = rb_str_new(0, 0);
> +	a->vec = &stub;

Can we just leave a->vec unset?  It's always set in fill_iovec anyways.

Static variables inside library function smells funny.

> +static void fill_iovec(struct io_args_v *a)
> +{
> +	unsigned long i;
> +	struct iovec *curvec;
> +
> +	a->iov_cnt = RARRAY_LEN(a->buf);
> +	a->iov_len = 0;
> +	if (a->iov_cnt == 0) return;
> +	if (a->iov_cnt > iov_max) a->iov_cnt = iov_max;
> +	rb_str_resize(a->vec_buf, sizeof(struct iovec) * a->iov_cnt);
> +	curvec = a->vec = (struct iovec*)RSTRING_PTR(a->vec_buf);


> +		/* lets limit total memory to write,
> +		 * but always take first string */
> +		next_len = a->iov_len + str_len;
> +		if (i && next_len > WRITEV_MEMLIMIT) {
> +		    a->iov_cnt = i;
> +		    break;

Use hard tabs like the rest of the file

> +static VALUE my_writev(VALUE io, VALUE str, int io_wait)
> +{
> +	struct io_args_v a;
> +	long n;
> +
> +	prepare_writev(&a, io, str);
> +	set_nonblocking(a.fd);
> +
> +	do {
> +		fill_iovec(&a);
> +#ifdef HAVE_WRITEV
> +		/* for big strings use library function */
> +		if (a.iov_len / WRITEV_IMPL_THRESHOLD > a.iov_cnt)
> +			n = (long)writev(a.fd, a.vec, a.iov_cnt);
> +		else
> +#endif
> +			n = (long)custom_writev(a.fd, a.vec, a.iov_cnt, a.iov_len);

Please no.  CPP conditionals inside functions is confusing, but CPP
conditionals mixing with C conditionals makes my eyes bleed.

How about something like this?

#ifdef HAVE_WRITEV
static ssize_t
real_writev(int fd, const struct iovec *vec, int cnt, size_t total)
{
	return writev(fd, vec, cnt);
}
#else
#  define real_writev(fd,vec,cnt,total) custom_writev((fd),(vec),(cnt),(total))
#endif